All of lore.kernel.org
 help / color / mirror / Atom feed
* [virtio-dev] [PATCH v11 0/3] admin: Access legacy registers using admin commands
@ 2023-07-06 21:27 ` Parav Pandit
  0 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

This short series introduces legacy registers access commands for the owner
group member access the legacy registers of the member VFs.
This short series introduces legacy region access commands by the group owner
device for its member devices.
Currently it is applicable to the PCI PF and VF devices. 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 split rows of admin opcode tables by a line
patch-2 fix section numbering
patch-3 add legacy region access commands

It uses the newly introduced administration command facility with 4 new
commands and a new optional command to query the legacy notification region.

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.

Overview:
---------
Above usecase requirements is solved by PCI PF group owner accessing
its group member PCI VFs legacy registers using an admin virtqueue of
the group owner PCI PF.

Two new admin virtqueue commands are added which read/write PCI VF
registers.

Software usage example:
-----------------------
One 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 | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
   Legacy region            Driver notification
    access                       |
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | 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 in the host.

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

Please review.

Alternatives considered:
========================
1. Exposing BAR0 as MMIO BAR that follows legacy registers template
Pros:
a. Kind of works with legacy drivers as some of them have used API
   which is agnostic to MMIO vs IOBAR.
b. Does not require hypervisor intervantion
Cons:
a. Device reset is extremely hard to implement in device at scale as
   driver does not wait for device reset completion
b. Device register width related problems persist that hypervisor if
   wishes, it cannot be fixed.

2. Accessing VF registers by tunneling it through new legacy PCI capability
Pros:
a. Self contained, but cannot work with future PCI SIOV devices
Cons:
a. Equally slow as AQ access
b. Still requires new capability for notification access
c. Requires hardware to build low level registers access which is not worth
   for long term future

3. Accessing VF notification region using new PF BAR
Cons:
a. Requires hardware to build new PCI steering logic per PF to forward
   notification from the PF to VF, requires double the amount of logic
   compared to today
b. Requires very large additional PF BAR whose size must be max_Vfs * BAR size.

4. Trapping CVQ, configuration region, LEGACY_HDR
Cons:
a. This does not fullfil the very basic requirement to not trap the
   1.x objects (configuration registers, vqs)
b. Requires feature negotiations mediation in hypervisor software
c. Requires constant device type specific knowledge in hypervisor driver
   (Does not scale for 30+ device types)

4. F_LEACY_HDR, F_WRITE_MAC
Cons:
a. Requires device support to have read/write mac address which is
   hard to implement on every member device.
b. such functionality is duplicate of existing cvq per device.
c. config space is only for the initialization specific purpose.
d. Requires mediation of 1.x objects, which is not good design.
e. Solves only for the net device.
Pros:
a. May work for nested env

conclusion for picking AQ approach:
==================================
1. Overall AQ based access is simpler to implement with combination of
   best from software and device so that legacy registers do not get baked
   in the device hardware
2. AQ allows hypervisor software to intercept legacy registers and make
   corrections if needed
3. Provides trade-off between performance, device complexity vs spec,
   while still maintaining passthrough mode for the VFs with minimal
   hypervisor intercepts only for legacy registers access
4. AQ mechanism is designed for accessing other member devices registers
   as noted in AQ submission, it utilizes the existing infrastructure over
   other alternatives.
5. Uses existing driver notification region similar to legacy notification
   saves hardware resources

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

---
changelog:
v10->v11:
- replaced tab with white spaces in read structure
- included pci fields along side other generic fields to avoid
  indirection
- merged pci conformance section
- avoid using definite in starting introduction
- replace 'all of the' with 'any of the'
- changed drivers notification normative to indicate use of
  NOTIFY_INFO command
- renamed NOTIFY_QUERY to NOTIFY_INFO name
- merged 4th patch with 3rd
- added normative line for notify_info command
- reworded notification region command description to be more verbose
- merged flags and owner field to indicate end of list
v9->v10:
- added white space at end of line
- addressed below comments from Cornelia
- fixed errors related to article
- hardwire to hardwires
- replaced various to all
- added hardwire to zero
- fixed requirements for administration virtqueue section
- added missing articles
- reworded description for notification query command
- grammar fixes
- addressed below comments from Michael
- added description for member group id setting
- reworded device and driver conformance statements
- opcode table description updated
- fixed label for device read command
- length alignment restriction text added
- data length described for read write commands
- notification description added and refined
- reworded text around command specific result and data field usage
v8->v9:
- add missing articles in notify query command
- replaced 'this notification' with 'such a notification'
- addressed below comments from Michael
- dropped 'Region' from the commands
- added 7 reserved pad bytes in config write commands
- rewrote from 'use following structure' to 'field' has the following
  struct..
- dropped mentioning to follow struct virtio_admin_cmd.
- added note about command limited to only sriov group type for now
- rewrote the description little differently
v7->v8:
- remove empty line at the end of file
- removed white space at the end
- addressed comments from Michael add link to pci
- renamed region to region_data
- made region_data width to be 16 bytes to cover for 8 bytes offset
- moved generic notification region related normative from pci to
  generic section
- addressed comments from Michael
- made bar offset 64-bit
- prefix legacy specific structure with _legacy
- moved generic normative from pci to generic section
- added link to virtio pci capabilities when referring to bar 0
- remove 'should' from generic description
v6->v7:
- addressed several comments from Michael
- use AQ command to query legacy notify region, dropped pci capability
  modifications
- moved most part of the text to the generic admin command section
- replace administrative to administration
- replace admin vq citation to admin commands
- added normatives for device and driver side
- made BAR0 to be not used at all when supporting legacy interface
- added normative around BAR0 and SR-IOV extended capability
- grammar corrections
v5->v6:
- fixed previous missed abbreviation of LCC and LD
- added text for the PCI capability for the group member device
v4->v5:
- split pci transport and generic command section to new patch
- removed multiple references to the VF
- written the description of the command as generic with member
  and group device terminology
- reflected many section names to remove VF
- split from pci transport specific patch
- split conformance to transport and generic sections
- written the description of the command as generic with member
  and group device terminology
- reflected many section names to remove VF
- rename fields from register to region
- avoided abbreviation for legacy, device and config
v3->v4:
- moved noted to the conformance section details in next patch
- removed queue notify address query AQ command on Michael's suggestion,
  though it is fine. Instead replaced with extending virtio_pci_notify_cap
  to indicate that legacy queue notifications can be done on the
  notification location
- fixed spelling errors
- replaced administrative virtqueue to administration virtqueue
- moved legacy interface normative references to legacy conformance
  section
v2->v3:
- added new patch to split raws of admin vq opcode table
- adddressed Jason and Michael's comment to split single register
  access command to common config and device specific commands.
- dropped the suggetion to introduce enable/disable command as
  admin command cap bit already covers it.
- added other alternative design considered and discussed in detail in v0, v1 and v2
v1->v2:
- addressed comments from Michael
- added theory of operation
- grammar corrections
- removed group fields description from individual commands as
  it is already present in generic section
- added endianness normative for legacy device registers region
- renamed the file to drop vf and add legacy prefix
- added overview in commit log
- renamed subsection to reflect command
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 (3):
  admin: Split opcode table rows with a line
  admin: Fix section numbering
  admin: Add group member legacy register access commands

 admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
 admin.tex                       |  24 ++-
 conformance.tex                 |   2 +
 3 files changed, 323 insertions(+), 5 deletions(-)
 create mode 100644 admin-cmds-legacy-interface.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] 18+ messages in thread

* [virtio-comment] [PATCH v11 0/3] admin: Access legacy registers using admin commands
@ 2023-07-06 21:27 ` Parav Pandit
  0 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

This short series introduces legacy registers access commands for the owner
group member access the legacy registers of the member VFs.
This short series introduces legacy region access commands by the group owner
device for its member devices.
Currently it is applicable to the PCI PF and VF devices. 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 split rows of admin opcode tables by a line
patch-2 fix section numbering
patch-3 add legacy region access commands

It uses the newly introduced administration command facility with 4 new
commands and a new optional command to query the legacy notification region.

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.

Overview:
---------
Above usecase requirements is solved by PCI PF group owner accessing
its group member PCI VFs legacy registers using an admin virtqueue of
the group owner PCI PF.

Two new admin virtqueue commands are added which read/write PCI VF
registers.

Software usage example:
-----------------------
One 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 | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
   Legacy region            Driver notification
    access                       |
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | 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 in the host.

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

Please review.

Alternatives considered:
========================
1. Exposing BAR0 as MMIO BAR that follows legacy registers template
Pros:
a. Kind of works with legacy drivers as some of them have used API
   which is agnostic to MMIO vs IOBAR.
b. Does not require hypervisor intervantion
Cons:
a. Device reset is extremely hard to implement in device at scale as
   driver does not wait for device reset completion
b. Device register width related problems persist that hypervisor if
   wishes, it cannot be fixed.

2. Accessing VF registers by tunneling it through new legacy PCI capability
Pros:
a. Self contained, but cannot work with future PCI SIOV devices
Cons:
a. Equally slow as AQ access
b. Still requires new capability for notification access
c. Requires hardware to build low level registers access which is not worth
   for long term future

3. Accessing VF notification region using new PF BAR
Cons:
a. Requires hardware to build new PCI steering logic per PF to forward
   notification from the PF to VF, requires double the amount of logic
   compared to today
b. Requires very large additional PF BAR whose size must be max_Vfs * BAR size.

4. Trapping CVQ, configuration region, LEGACY_HDR
Cons:
a. This does not fullfil the very basic requirement to not trap the
   1.x objects (configuration registers, vqs)
b. Requires feature negotiations mediation in hypervisor software
c. Requires constant device type specific knowledge in hypervisor driver
   (Does not scale for 30+ device types)

4. F_LEACY_HDR, F_WRITE_MAC
Cons:
a. Requires device support to have read/write mac address which is
   hard to implement on every member device.
b. such functionality is duplicate of existing cvq per device.
c. config space is only for the initialization specific purpose.
d. Requires mediation of 1.x objects, which is not good design.
e. Solves only for the net device.
Pros:
a. May work for nested env

conclusion for picking AQ approach:
==================================
1. Overall AQ based access is simpler to implement with combination of
   best from software and device so that legacy registers do not get baked
   in the device hardware
2. AQ allows hypervisor software to intercept legacy registers and make
   corrections if needed
3. Provides trade-off between performance, device complexity vs spec,
   while still maintaining passthrough mode for the VFs with minimal
   hypervisor intercepts only for legacy registers access
4. AQ mechanism is designed for accessing other member devices registers
   as noted in AQ submission, it utilizes the existing infrastructure over
   other alternatives.
5. Uses existing driver notification region similar to legacy notification
   saves hardware resources

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

---
changelog:
v10->v11:
- replaced tab with white spaces in read structure
- included pci fields along side other generic fields to avoid
  indirection
- merged pci conformance section
- avoid using definite in starting introduction
- replace 'all of the' with 'any of the'
- changed drivers notification normative to indicate use of
  NOTIFY_INFO command
- renamed NOTIFY_QUERY to NOTIFY_INFO name
- merged 4th patch with 3rd
- added normative line for notify_info command
- reworded notification region command description to be more verbose
- merged flags and owner field to indicate end of list
v9->v10:
- added white space at end of line
- addressed below comments from Cornelia
- fixed errors related to article
- hardwire to hardwires
- replaced various to all
- added hardwire to zero
- fixed requirements for administration virtqueue section
- added missing articles
- reworded description for notification query command
- grammar fixes
- addressed below comments from Michael
- added description for member group id setting
- reworded device and driver conformance statements
- opcode table description updated
- fixed label for device read command
- length alignment restriction text added
- data length described for read write commands
- notification description added and refined
- reworded text around command specific result and data field usage
v8->v9:
- add missing articles in notify query command
- replaced 'this notification' with 'such a notification'
- addressed below comments from Michael
- dropped 'Region' from the commands
- added 7 reserved pad bytes in config write commands
- rewrote from 'use following structure' to 'field' has the following
  struct..
- dropped mentioning to follow struct virtio_admin_cmd.
- added note about command limited to only sriov group type for now
- rewrote the description little differently
v7->v8:
- remove empty line at the end of file
- removed white space at the end
- addressed comments from Michael add link to pci
- renamed region to region_data
- made region_data width to be 16 bytes to cover for 8 bytes offset
- moved generic notification region related normative from pci to
  generic section
- addressed comments from Michael
- made bar offset 64-bit
- prefix legacy specific structure with _legacy
- moved generic normative from pci to generic section
- added link to virtio pci capabilities when referring to bar 0
- remove 'should' from generic description
v6->v7:
- addressed several comments from Michael
- use AQ command to query legacy notify region, dropped pci capability
  modifications
- moved most part of the text to the generic admin command section
- replace administrative to administration
- replace admin vq citation to admin commands
- added normatives for device and driver side
- made BAR0 to be not used at all when supporting legacy interface
- added normative around BAR0 and SR-IOV extended capability
- grammar corrections
v5->v6:
- fixed previous missed abbreviation of LCC and LD
- added text for the PCI capability for the group member device
v4->v5:
- split pci transport and generic command section to new patch
- removed multiple references to the VF
- written the description of the command as generic with member
  and group device terminology
- reflected many section names to remove VF
- split from pci transport specific patch
- split conformance to transport and generic sections
- written the description of the command as generic with member
  and group device terminology
- reflected many section names to remove VF
- rename fields from register to region
- avoided abbreviation for legacy, device and config
v3->v4:
- moved noted to the conformance section details in next patch
- removed queue notify address query AQ command on Michael's suggestion,
  though it is fine. Instead replaced with extending virtio_pci_notify_cap
  to indicate that legacy queue notifications can be done on the
  notification location
- fixed spelling errors
- replaced administrative virtqueue to administration virtqueue
- moved legacy interface normative references to legacy conformance
  section
v2->v3:
- added new patch to split raws of admin vq opcode table
- adddressed Jason and Michael's comment to split single register
  access command to common config and device specific commands.
- dropped the suggetion to introduce enable/disable command as
  admin command cap bit already covers it.
- added other alternative design considered and discussed in detail in v0, v1 and v2
v1->v2:
- addressed comments from Michael
- added theory of operation
- grammar corrections
- removed group fields description from individual commands as
  it is already present in generic section
- added endianness normative for legacy device registers region
- renamed the file to drop vf and add legacy prefix
- added overview in commit log
- renamed subsection to reflect command
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 (3):
  admin: Split opcode table rows with a line
  admin: Fix section numbering
  admin: Add group member legacy register access commands

 admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
 admin.tex                       |  24 ++-
 conformance.tex                 |   2 +
 3 files changed, 323 insertions(+), 5 deletions(-)
 create mode 100644 admin-cmds-legacy-interface.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] 18+ messages in thread

* [virtio-dev] [PATCH v11 1/3] admin: Split opcode table rows with a line
  2023-07-06 21:27 ` [virtio-comment] " Parav Pandit
@ 2023-07-06 21:27   ` Parav Pandit
  -1 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

Currently all opcode appears to be in a single row.
Separate them with a line similar to other tables.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

---
changelog:
v2->v3:
- new patch
---
 admin.tex | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/admin.tex b/admin.tex
index 2efd4d7..e51f9e6 100644
--- a/admin.tex
+++ b/admin.tex
@@ -114,7 +114,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 opcode & Name & Command Description \\
 \hline \hline
 0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+\hline
 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
+\hline
 0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
-- 
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] 18+ messages in thread

* [virtio-comment] [PATCH v11 1/3] admin: Split opcode table rows with a line
@ 2023-07-06 21:27   ` Parav Pandit
  0 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

Currently all opcode appears to be in a single row.
Separate them with a line similar to other tables.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>

---
changelog:
v2->v3:
- new patch
---
 admin.tex | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/admin.tex b/admin.tex
index 2efd4d7..e51f9e6 100644
--- a/admin.tex
+++ b/admin.tex
@@ -114,7 +114,9 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 opcode & Name & Command Description \\
 \hline \hline
 0x0000 & VIRTIO_ADMIN_CMD_LIST_QUERY & Provides to driver list of commands supported for this group type    \\
+\hline
 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
+\hline
 0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
-- 
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] 18+ messages in thread

* [virtio-dev] [PATCH v11 2/3] admin: Fix section numbering
  2023-07-06 21:27 ` [virtio-comment] " Parav Pandit
@ 2023-07-06 21:27   ` Parav Pandit
  -1 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

Requirements are put one additional level down. Fix it.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
changelog:
v9->v10:
- addressed comments from Cornelia
- fixed requirements for administration virtqueue section
v4->v5:
- new patch
---
 admin.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/admin.tex b/admin.tex
index e51f9e6..b0a1a91 100644
--- a/admin.tex
+++ b/admin.tex
@@ -286,7 +286,7 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 supporting multiple group types, the list of supported commands
 might differ between different group types.
 
-\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+\devicenormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
 The device MUST validate \field{opcode}, \field{group_type} and
 \field{group_member_id}, and if any of these has an invalid or
@@ -378,7 +378,7 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{VF Enable} refer to registers within the SR-IOV Extended
 Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
 
-\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+\drivernormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
 The driver MAY discover whether device supports a specific group type
 by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
@@ -506,7 +506,7 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
 tail of a structure, with the driver/device using the full
 structure without concern for versioning.
 
-\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+\devicenormative{\subsection}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
 
 The device MUST support device-readable and device-writeable buffers
 shorter than described in this specification, by
@@ -555,7 +555,7 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
 or VIRTIO_ADMIN_STATUS_ENOMEM, then the command MUST NOT
 have any side effects, making it safe to retry.
 
-\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+\drivernormative{\subsection}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
 
 The driver MAY supply device-readable or device-writeable parts
 of \field{struct virtio_admin_cmd} that are longer than described in
-- 
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] 18+ messages in thread

* [virtio-comment] [PATCH v11 2/3] admin: Fix section numbering
@ 2023-07-06 21:27   ` Parav Pandit
  0 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

Requirements are put one additional level down. Fix it.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
changelog:
v9->v10:
- addressed comments from Cornelia
- fixed requirements for administration virtqueue section
v4->v5:
- new patch
---
 admin.tex | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/admin.tex b/admin.tex
index e51f9e6..b0a1a91 100644
--- a/admin.tex
+++ b/admin.tex
@@ -286,7 +286,7 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 supporting multiple group types, the list of supported commands
 might differ between different group types.
 
-\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+\devicenormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
 The device MUST validate \field{opcode}, \field{group_type} and
 \field{group_member_id}, and if any of these has an invalid or
@@ -378,7 +378,7 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \field{VF Enable} refer to registers within the SR-IOV Extended
 Capability as specified by \hyperref[intro:PCIe]{[PCIe]}.
 
-\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
+\drivernormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
 The driver MAY discover whether device supports a specific group type
 by issuing VIRTIO_ADMIN_CMD_LIST_QUERY with the matching
@@ -506,7 +506,7 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
 tail of a structure, with the driver/device using the full
 structure without concern for versioning.
 
-\devicenormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+\devicenormative{\subsection}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
 
 The device MUST support device-readable and device-writeable buffers
 shorter than described in this specification, by
@@ -555,7 +555,7 @@ \section{Administration Virtqueues}\label{sec:Basic Facilities of a Virtio Devic
 or VIRTIO_ADMIN_STATUS_ENOMEM, then the command MUST NOT
 have any side effects, making it safe to retry.
 
-\drivernormative{\paragraph}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
+\drivernormative{\subsection}{Group administration commands}{Basic Facilities of a Virtio Device / Administration virtqueues}
 
 The driver MAY supply device-readable or device-writeable parts
 of \field{struct virtio_admin_cmd} that are longer than described in
-- 
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] 18+ messages in thread

* [virtio-dev] [PATCH v11 3/3] admin: Add group member legacy register access commands
  2023-07-06 21:27 ` [virtio-comment] " Parav Pandit
@ 2023-07-06 21:27   ` Parav Pandit
  -1 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

Introduce group member legacy common configuration and legacy device
configuration access read/write commands.

Group member legacy registers access commands enable group owner driver
software to access legacy registers on behalf of the guest virtual
machine.

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.

Overview:
=========
Above usecase requirements is solved by PCI PF group owner accessing
its group member PCI VFs legacy registers using the administration
commands of the group owner PCI PF.

Two types of administration commands are added which read/write PCI VF
registers.

Software usage example:
=======================

1. One 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 | |
|   | forwarder    |     |                 | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
   Config region                 |
     access                Driver notifications
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | PCI VF device A |
  | | AQ  |-------------+---->+-------------+ |
  | +-----+         |   |   | | legacy regs | |
  | PCI PF device   |   |   | +-------------+ |
  +-----------------+   |   +-----------------+
                        |
                        |   +----+------------+
                        |   | PCI VF device N |
                        +---->+-------------+ |
                            | | legacy regs | |
                            | +-------------+ |
                            +-----------------+

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

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

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v10->v11:
- replaced tab with white spaces in read structure
- included pci fields along side other generic fields to avoid
  indirection
- merged pci conformance section
- avoid using definite in starting introduction
- replace 'all of the' with 'any of the'
- changed drivers notification normative to indicate use of
  NOTIFY_INFO command
- renamed NOTIFY_QUERY to NOTIFY_INFO name
- merged 4th patch with 3rd
- added normative line for notify_info command
- reworded notification region command description to be more verbose
- merged flags and owner field to indicate end of list
v9->v10:
- added white space at end of line
- addressed below comments from Cornelia
- added missing articles
- reworded description for notification query command
- grammar fixes
- addressed below comments from Michael
- added description for member group id setting
- reworded device and driver conformance statements
- opcode table description updated
- fixed label for device read command
- length alignment restriction text added
- data length described for read write commands
- notification description added and refined
- reworded text around command specific result and data field usage
v8->v9:
- add missing articles in notify query command
- replaced 'this notification' with 'such a notification'
- addressed below comments from Michael
- dropped 'Region' from the commands
- added 7 reserved pad bytes in config write commands
- rewrote from 'use following structure' to 'field' has the following
  struct..
- dropped mentioning to follow struct virtio_admin_cmd.
- added note about command limited to only sriov group type for now
- rewrote the description little differently
v7->v8:
- remove empty line at the end of file
- removed white space at the end
- addressed comments from Michael add link to pci
- renamed region to region_data
- made region_data width to be 16 bytes to cover for 8 bytes offset
- moved generic notification region related normative from pci to
  generic section
v6->v7:
- changed administrative to administration
- renamed admin-access.tex to admin-interface.tex
- large rewrite ad generic admin commands instead of pci
- added theory of operation section
- added driver notification region query command
v5->v6:
- fixed previous missed abbreviation of LCC and LD
v4->v5:
- split from pci transport specific patch
- split conformance to transport and generic sections
- written the description of the command as generic with member
  and group device terminology
- reflected many section names to remove VF
- rename fields from register to region
- avoided abbreviation for legacy, device and config
---
 admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
 admin.tex                       |  14 +-
 conformance.tex                 |   2 +
 3 files changed, 317 insertions(+), 1 deletion(-)
 create mode 100644 admin-cmds-legacy-interface.tex

diff --git a/admin-cmds-legacy-interface.tex b/admin-cmds-legacy-interface.tex
new file mode 100644
index 0000000..1ecb5af
--- /dev/null
+++ b/admin-cmds-legacy-interface.tex
@@ -0,0 +1,302 @@
+\subsubsection{Legacy Interfaces}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group
+administration commands / Legacy Interface}
+
+In some systems, there is a need to support utilizing a legacy driver with
+a device that does not directly support the legacy interface. In such scenarios,
+a group owner device can provide the legacy interface functionality for the
+group member devices. The driver of an owner device can then access the legacy
+interface of a member device on behalf of the legacy member device driver.
+
+For example, with the SR-IOV group type, group members (VFs) can not present
+the legacy interface in an I/O BAR in BAR0 as expected by the legacy pci driver.
+If the legacy driver is running inside a virtual machine, the hypervisor
+executing the virtual machine can present a virtual device with an I/O BAR in
+BAR0. The hypervisor intercepts the legacy driver accesses to this I/O BAR and
+forwards them to the group owner device (PF) using group administration commands.
+
+The following commands support such legacy interface functionality:
+
+\begin{enumerate}
+\item Legacy Common Configuration Write Command
+\item Legacy Common Configuration Read Command
+\item Legacy Device Configuration Write Command
+\item Legacy Device Configuration Read Command
+\end{enumerate}
+
+These commands are currently only defined for the SR-IOV group type and
+have, generally, the same effect as member device accesses through a legacy
+interface listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} except that little endian format is assumed unconditionally.
+
+\paragraph{Legacy Common Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group
+administration commands / Legacy Interface / Legacy Common Configuration Write Command}
+
+This command has the same effect as writing into the virtio common configuration
+structure through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_common_cfg_wr_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_common_cfg_wr_data {
+        u8 offset; /* Starting byte offset within the common configuration structure to write */
+        u8 reserved[7];
+        u8 data[];
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE the driver sets
+\field{opcode} to 0x2.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. The driver sets \field{offset} which refers to the offset to write within the
+virtio common configuration structure, and excluding the device-specific configuration.
+The length of the data to write is simply the length of \field{data}.
+
+No length or alignment restrictions are placed on the value of the
+\field{offset} and the length of the \field{data}, except that the resulting
+access refers to a single field and is completely within the virtio common
+configuration structure, excluding the device-specific configuration.
+
+This command has no command specific result.
+
+\paragraph{Legacy Common Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Common Configuration Read Command}
+
+This command has the same effect as reading from the virtio common configuration
+structure through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_common_cfg_rd_data {
+        u8 offset; /* Starting byte offset within the common configuration structure to read */
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ the driver sets
+\field{opcode} to 0x3.
+The driver sets \field{offset} which refers to the offset to read from the
+virtio common configuration structure, and excluding the device-specific
+configuration.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_common_cfg_rd_result {
+        u8 data[];
+};
+\end{lstlisting}
+
+When the command completes successfully, \field{command_specific_result}
+is in the format of \field{struct virtio_admin_cmd_legacy_common_cfg_rd_result}
+returned by the device. The length of the data read is simply the length of
+\field{data}.
+
+\paragraph{Legacy Device Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Write Command}
+
+This command has the same effect as writing into the virtio device-specific
+configuration through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_dev_reg_wr_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_dev_reg_wr_data {
+        u8 offset; /* Starting byte offset within the device-specific configuration to write */
+        u8 reserved[7];
+        u8 data[];
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
+\field{opcode} to 0x4.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. The driver sets \field{offset} which refers to the offset to write
+within the virtio device-specific configuration. The length of the data to write
+is simply the length of \field{data}.
+
+No length or alignment restrictions are placed on the value of the
+\field{offset} and the length of the \field{data}, except that the resulting
+access refers to a single field and is completely within the device-specific
+configuration.
+
+This command has no command specific result.
+
+\paragraph{Legacy Device Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Read Command}
+
+This command has the same effect as reading from the virtio device-specific
+configuration through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_dev_cfg_rd_data {
+        u8 offset; /* Starting byte offset within the device-specific configuration to read */
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
+\field{opcode} to 0x5.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. The driver sets \field{offset} which refers to the offset to read from
+the virtio device-specific configuration.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_dev_reg_rd_result {
+        u8 data[];
+};
+\end{lstlisting}
+
+When the command completes successfully, \field{command_specific_result} is in
+the format of \field{struct virtio_admin_cmd_legacy_dev_reg_rd_result}
+returned by the device.
+
+The length of the data read is simply the length of \field{data}.
+
+\paragraph{Legacy Driver Notification}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Driver Notifications}
+
+The driver of the owner device can send a driver notification to the member
+device by executing VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE with the
+\field{offset} matching \field{Queue Notify} and the \field{data} containing
+the virtqueue index to be notified.
+
+However, as many administration commands are used for slow path configuration,
+a separate fast path mechanism for such notifications is desired. For the
+SR-IOV group type, the optional command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
+addresses this need by returning to the driver one or more addresses which to
+be used to send such driver notifications. The member driver in the hypervisor
+uses the notification location supplied in
+the VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO result. The member driver in the
+hypervisor which intercepts I/O BAR writes, for the \field{Queue Notify} writes,
+the member driver performs memory or I/O access directly in the supplied
+notification location instead of executing slow administration command.
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO the driver sets
+\field{opcode} to 0x6.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. This command does not use \field{command_specific_data}.
+
+This command is currently only defined for the SR-IOV group type. When the
+device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner
+device hardwires VF BAR0 to zero in the SR-IOV Extended capability and the
+group member device does not use PCI BAR0 in any of the Virtio PCI capabilities
+listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Capabilities}.
+
+\begin{lstlisting}
+struct virtio_pci_legacy_notify_info {
+        u8 flags;  /* 0 = end of list, 1 = owner device, 2 = member device */
+        u8 bar;    /* BAR of the member or owner device */
+        u8 padding[7];
+        le64 offset; /* Offset within bar. */
+};
+\end{lstlisting}
+
+\field{flags} value of 0x1 indicates that the notification region is of owner
+device, value of 0x2 indicates that the notification region is of member
+device, the value of 0 indicates that all the entries starting from that entry
+are invalid entries in \field{entries}. All other values in \field{flags} are
+reserved.
+
+\begin{lstlisting}
+union virtio_admin_cmd_legacy_notify_info {
+        u8 data[16];
+	struct virtio_pci_legacy_notify_info pci_info;
+};
+
+struct virtio_admin_cmd_legacy_notify_info_result {
+	struct virtio_virtio_admin_cmd_legacy_notify_info entries[4];
+};
+\end{lstlisting}
+
+When the command completes successfully, \field{command_specific_result} is in
+the format of \field{struct virtio_admin_cmd_legacy_notify_info_result}. The
+driver can use any entry when multiple entries are supplied by the device.
+
+\devicenormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
+
+A device MUST either support all of, or none of
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
+
+For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the device MUST decode and
+encode (respectively) the value of the \field{data} using the little-endian
+format.
+
+The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands where the value of the
+\field{offset} and the length of the \field{data} does not refer to a
+single field or is not completely within the virtio common configuration
+structure excluding the device-specific configuration.
+
+The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands where the value of the
+\field{offset} and the length of the \field{data} does not refer to a
+single field or is not completely within the device-specific configuration.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE MUST have the same effect
+as writing into the virtio common configuration structure through the legacy
+interface.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ MUST have the same effect as
+reading from the virtio common configuration structure through the legacy
+interface.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE MUST have the same effect as
+writing into the virtio device configuration structure through the legacy
+interface.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ MUST have the same effect as
+reading from the virtio device configuration structure through the legacy
+interface.
+
+If the device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO it MUST
+also support all of VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
+
+The device MAY support VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO with entries
+of the owner device or the member device or both of them.
+
+For the SR-IOV group type, when the owner device supports
+VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner device MUST
+hardwire VF BAR0 to zero in the SR-IOV Extended capability.
+
+For the SR-IOV group type, when the owner device supports
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
+commands, the owner device and the group member device SHOULD follow the rules
+for the PCI Revision ID and Subsystem Device ID of the non-transitional devices
+documented in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
+
+For the SR-IOV group type, when the owner device supports
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
+commands, the owner device SHOULD follow the rules for the PCI Device ID of the non-transitional
+devices documented in section
+\ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
+
+\drivernormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
+
+For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver MUST encode and
+decode (respectively) the value of the \field{data} using the little-endian
+format.
+
+For VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands the driver SHOULD set
+\field{offset} and the length of the \field{data} to refer to a single
+field within the virtio common configuration structure excluding
+the device-specific configuration.
+
+For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver SHOULD set
+\field{offset} and the length of the \field{data} to refer to a single
+field within the virtio device-specific configuration.
+
+If VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command is supported, the group member
+driver SHOULD use the notification region to send a driver notification to the
+device.
+
+When the device reports zero in \field{flags} in
+\field{struct virtio_pci_legacy_notify_info} for the entry, the driver must
+ignore all other fields of \field{struct virtio_pci_legacy_notify_info}.
diff --git a/admin.tex b/admin.tex
index b0a1a91..0803c26 100644
--- a/admin.tex
+++ b/admin.tex
@@ -117,7 +117,17 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \hline
 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
 \hline
-0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+0x0002 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE & Writes into the legacy common configuration structure \\
+\hline
+0x0003 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ & Reads from the legacy common configuration structure  \\
+\hline
+0x0004 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE & Writes into the legacy device configuration structure \\
+\hline
+0x0005 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ & Reads into the legacy device configuration structure \\
+\hline
+0x0006 & VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO & Query the notification region information \\
+\hline
+0x0007 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
 \hline
@@ -286,6 +296,8 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 supporting multiple group types, the list of supported commands
 might differ between different group types.
 
+\input{admin-cmds-legacy-interface.tex}
+
 \devicenormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
 The device MUST validate \field{opcode}, \field{group_type} and
diff --git a/conformance.tex b/conformance.tex
index 01ccd69..dc00e84 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -260,6 +260,8 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
 \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
 \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
+\item Section \ref{devicenormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
+\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
 \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
 \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
 \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
-- 
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] 18+ messages in thread

* [virtio-comment] [PATCH v11 3/3] admin: Add group member legacy register access commands
@ 2023-07-06 21:27   ` Parav Pandit
  0 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-06 21:27 UTC (permalink / raw)
  To: virtio-comment, mst, cohuck, david.edmondson
  Cc: virtio-dev, sburla, jasowang, yishaih, maorg, shahafs, Parav Pandit

Introduce group member legacy common configuration and legacy device
configuration access read/write commands.

Group member legacy registers access commands enable group owner driver
software to access legacy registers on behalf of the guest virtual
machine.

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.

Overview:
=========
Above usecase requirements is solved by PCI PF group owner accessing
its group member PCI VFs legacy registers using the administration
commands of the group owner PCI PF.

Two types of administration commands are added which read/write PCI VF
registers.

Software usage example:
=======================

1. One 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 | |
|   | forwarder    |     |                 | |
|   +--------------+     +-----------------+ |
|                                            |
+------+-------------------------+-----------+
       |                         |
   Config region                 |
     access                Driver notifications
       |                         |
  +----+------------+       +----+------------+
  | +-----+         |       | PCI VF device A |
  | | AQ  |-------------+---->+-------------+ |
  | +-----+         |   |   | | legacy regs | |
  | PCI PF device   |   |   | +-------------+ |
  +-----------------+   |   +-----------------+
                        |
                        |   +----+------------+
                        |   | PCI VF device N |
                        +---->+-------------+ |
                            | | legacy regs | |
                            | +-------------+ |
                            +-----------------+

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

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

Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
Signed-off-by: Parav Pandit <parav@nvidia.com>
---
changelog:
v10->v11:
- replaced tab with white spaces in read structure
- included pci fields along side other generic fields to avoid
  indirection
- merged pci conformance section
- avoid using definite in starting introduction
- replace 'all of the' with 'any of the'
- changed drivers notification normative to indicate use of
  NOTIFY_INFO command
- renamed NOTIFY_QUERY to NOTIFY_INFO name
- merged 4th patch with 3rd
- added normative line for notify_info command
- reworded notification region command description to be more verbose
- merged flags and owner field to indicate end of list
v9->v10:
- added white space at end of line
- addressed below comments from Cornelia
- added missing articles
- reworded description for notification query command
- grammar fixes
- addressed below comments from Michael
- added description for member group id setting
- reworded device and driver conformance statements
- opcode table description updated
- fixed label for device read command
- length alignment restriction text added
- data length described for read write commands
- notification description added and refined
- reworded text around command specific result and data field usage
v8->v9:
- add missing articles in notify query command
- replaced 'this notification' with 'such a notification'
- addressed below comments from Michael
- dropped 'Region' from the commands
- added 7 reserved pad bytes in config write commands
- rewrote from 'use following structure' to 'field' has the following
  struct..
- dropped mentioning to follow struct virtio_admin_cmd.
- added note about command limited to only sriov group type for now
- rewrote the description little differently
v7->v8:
- remove empty line at the end of file
- removed white space at the end
- addressed comments from Michael add link to pci
- renamed region to region_data
- made region_data width to be 16 bytes to cover for 8 bytes offset
- moved generic notification region related normative from pci to
  generic section
v6->v7:
- changed administrative to administration
- renamed admin-access.tex to admin-interface.tex
- large rewrite ad generic admin commands instead of pci
- added theory of operation section
- added driver notification region query command
v5->v6:
- fixed previous missed abbreviation of LCC and LD
v4->v5:
- split from pci transport specific patch
- split conformance to transport and generic sections
- written the description of the command as generic with member
  and group device terminology
- reflected many section names to remove VF
- rename fields from register to region
- avoided abbreviation for legacy, device and config
---
 admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
 admin.tex                       |  14 +-
 conformance.tex                 |   2 +
 3 files changed, 317 insertions(+), 1 deletion(-)
 create mode 100644 admin-cmds-legacy-interface.tex

diff --git a/admin-cmds-legacy-interface.tex b/admin-cmds-legacy-interface.tex
new file mode 100644
index 0000000..1ecb5af
--- /dev/null
+++ b/admin-cmds-legacy-interface.tex
@@ -0,0 +1,302 @@
+\subsubsection{Legacy Interfaces}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group
+administration commands / Legacy Interface}
+
+In some systems, there is a need to support utilizing a legacy driver with
+a device that does not directly support the legacy interface. In such scenarios,
+a group owner device can provide the legacy interface functionality for the
+group member devices. The driver of an owner device can then access the legacy
+interface of a member device on behalf of the legacy member device driver.
+
+For example, with the SR-IOV group type, group members (VFs) can not present
+the legacy interface in an I/O BAR in BAR0 as expected by the legacy pci driver.
+If the legacy driver is running inside a virtual machine, the hypervisor
+executing the virtual machine can present a virtual device with an I/O BAR in
+BAR0. The hypervisor intercepts the legacy driver accesses to this I/O BAR and
+forwards them to the group owner device (PF) using group administration commands.
+
+The following commands support such legacy interface functionality:
+
+\begin{enumerate}
+\item Legacy Common Configuration Write Command
+\item Legacy Common Configuration Read Command
+\item Legacy Device Configuration Write Command
+\item Legacy Device Configuration Read Command
+\end{enumerate}
+
+These commands are currently only defined for the SR-IOV group type and
+have, generally, the same effect as member device accesses through a legacy
+interface listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} except that little endian format is assumed unconditionally.
+
+\paragraph{Legacy Common Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group
+administration commands / Legacy Interface / Legacy Common Configuration Write Command}
+
+This command has the same effect as writing into the virtio common configuration
+structure through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_common_cfg_wr_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_common_cfg_wr_data {
+        u8 offset; /* Starting byte offset within the common configuration structure to write */
+        u8 reserved[7];
+        u8 data[];
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE the driver sets
+\field{opcode} to 0x2.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. The driver sets \field{offset} which refers to the offset to write within the
+virtio common configuration structure, and excluding the device-specific configuration.
+The length of the data to write is simply the length of \field{data}.
+
+No length or alignment restrictions are placed on the value of the
+\field{offset} and the length of the \field{data}, except that the resulting
+access refers to a single field and is completely within the virtio common
+configuration structure, excluding the device-specific configuration.
+
+This command has no command specific result.
+
+\paragraph{Legacy Common Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Common Configuration Read Command}
+
+This command has the same effect as reading from the virtio common configuration
+structure through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_common_cfg_rd_data {
+        u8 offset; /* Starting byte offset within the common configuration structure to read */
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ the driver sets
+\field{opcode} to 0x3.
+The driver sets \field{offset} which refers to the offset to read from the
+virtio common configuration structure, and excluding the device-specific
+configuration.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_common_cfg_rd_result {
+        u8 data[];
+};
+\end{lstlisting}
+
+When the command completes successfully, \field{command_specific_result}
+is in the format of \field{struct virtio_admin_cmd_legacy_common_cfg_rd_result}
+returned by the device. The length of the data read is simply the length of
+\field{data}.
+
+\paragraph{Legacy Device Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Write Command}
+
+This command has the same effect as writing into the virtio device-specific
+configuration through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_dev_reg_wr_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_dev_reg_wr_data {
+        u8 offset; /* Starting byte offset within the device-specific configuration to write */
+        u8 reserved[7];
+        u8 data[];
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
+\field{opcode} to 0x4.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. The driver sets \field{offset} which refers to the offset to write
+within the virtio device-specific configuration. The length of the data to write
+is simply the length of \field{data}.
+
+No length or alignment restrictions are placed on the value of the
+\field{offset} and the length of the \field{data}, except that the resulting
+access refers to a single field and is completely within the device-specific
+configuration.
+
+This command has no command specific result.
+
+\paragraph{Legacy Device Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Read Command}
+
+This command has the same effect as reading from the virtio device-specific
+configuration through the legacy interface. The \field{command_specific_data} is in
+the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
+the access to be performed.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_dev_cfg_rd_data {
+        u8 offset; /* Starting byte offset within the device-specific configuration to read */
+};
+\end{lstlisting}
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
+\field{opcode} to 0x5.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. The driver sets \field{offset} which refers to the offset to read from
+the virtio device-specific configuration.
+
+\begin{lstlisting}
+struct virtio_admin_cmd_legacy_dev_reg_rd_result {
+        u8 data[];
+};
+\end{lstlisting}
+
+When the command completes successfully, \field{command_specific_result} is in
+the format of \field{struct virtio_admin_cmd_legacy_dev_reg_rd_result}
+returned by the device.
+
+The length of the data read is simply the length of \field{data}.
+
+\paragraph{Legacy Driver Notification}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Driver Notifications}
+
+The driver of the owner device can send a driver notification to the member
+device by executing VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE with the
+\field{offset} matching \field{Queue Notify} and the \field{data} containing
+the virtqueue index to be notified.
+
+However, as many administration commands are used for slow path configuration,
+a separate fast path mechanism for such notifications is desired. For the
+SR-IOV group type, the optional command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
+addresses this need by returning to the driver one or more addresses which to
+be used to send such driver notifications. The member driver in the hypervisor
+uses the notification location supplied in
+the VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO result. The member driver in the
+hypervisor which intercepts I/O BAR writes, for the \field{Queue Notify} writes,
+the member driver performs memory or I/O access directly in the supplied
+notification location instead of executing slow administration command.
+
+For the command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO the driver sets
+\field{opcode} to 0x6.
+The driver sets \field{group_member_id} which refers to the member device to be
+accessed. This command does not use \field{command_specific_data}.
+
+This command is currently only defined for the SR-IOV group type. When the
+device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner
+device hardwires VF BAR0 to zero in the SR-IOV Extended capability and the
+group member device does not use PCI BAR0 in any of the Virtio PCI capabilities
+listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Capabilities}.
+
+\begin{lstlisting}
+struct virtio_pci_legacy_notify_info {
+        u8 flags;  /* 0 = end of list, 1 = owner device, 2 = member device */
+        u8 bar;    /* BAR of the member or owner device */
+        u8 padding[7];
+        le64 offset; /* Offset within bar. */
+};
+\end{lstlisting}
+
+\field{flags} value of 0x1 indicates that the notification region is of owner
+device, value of 0x2 indicates that the notification region is of member
+device, the value of 0 indicates that all the entries starting from that entry
+are invalid entries in \field{entries}. All other values in \field{flags} are
+reserved.
+
+\begin{lstlisting}
+union virtio_admin_cmd_legacy_notify_info {
+        u8 data[16];
+	struct virtio_pci_legacy_notify_info pci_info;
+};
+
+struct virtio_admin_cmd_legacy_notify_info_result {
+	struct virtio_virtio_admin_cmd_legacy_notify_info entries[4];
+};
+\end{lstlisting}
+
+When the command completes successfully, \field{command_specific_result} is in
+the format of \field{struct virtio_admin_cmd_legacy_notify_info_result}. The
+driver can use any entry when multiple entries are supplied by the device.
+
+\devicenormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
+
+A device MUST either support all of, or none of
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
+
+For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the device MUST decode and
+encode (respectively) the value of the \field{data} using the little-endian
+format.
+
+The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands where the value of the
+\field{offset} and the length of the \field{data} does not refer to a
+single field or is not completely within the virtio common configuration
+structure excluding the device-specific configuration.
+
+The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands where the value of the
+\field{offset} and the length of the \field{data} does not refer to a
+single field or is not completely within the device-specific configuration.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE MUST have the same effect
+as writing into the virtio common configuration structure through the legacy
+interface.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ MUST have the same effect as
+reading from the virtio common configuration structure through the legacy
+interface.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE MUST have the same effect as
+writing into the virtio device configuration structure through the legacy
+interface.
+
+The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ MUST have the same effect as
+reading from the virtio device configuration structure through the legacy
+interface.
+
+If the device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO it MUST
+also support all of VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
+
+The device MAY support VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO with entries
+of the owner device or the member device or both of them.
+
+For the SR-IOV group type, when the owner device supports
+VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner device MUST
+hardwire VF BAR0 to zero in the SR-IOV Extended capability.
+
+For the SR-IOV group type, when the owner device supports
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
+commands, the owner device and the group member device SHOULD follow the rules
+for the PCI Revision ID and Subsystem Device ID of the non-transitional devices
+documented in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
+
+For the SR-IOV group type, when the owner device supports
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
+commands, the owner device SHOULD follow the rules for the PCI Device ID of the non-transitional
+devices documented in section
+\ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
+
+\drivernormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
+
+For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver MUST encode and
+decode (respectively) the value of the \field{data} using the little-endian
+format.
+
+For VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands the driver SHOULD set
+\field{offset} and the length of the \field{data} to refer to a single
+field within the virtio common configuration structure excluding
+the device-specific configuration.
+
+For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
+VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver SHOULD set
+\field{offset} and the length of the \field{data} to refer to a single
+field within the virtio device-specific configuration.
+
+If VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command is supported, the group member
+driver SHOULD use the notification region to send a driver notification to the
+device.
+
+When the device reports zero in \field{flags} in
+\field{struct virtio_pci_legacy_notify_info} for the entry, the driver must
+ignore all other fields of \field{struct virtio_pci_legacy_notify_info}.
diff --git a/admin.tex b/admin.tex
index b0a1a91..0803c26 100644
--- a/admin.tex
+++ b/admin.tex
@@ -117,7 +117,17 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 \hline
 0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
 \hline
-0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
+0x0002 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE & Writes into the legacy common configuration structure \\
+\hline
+0x0003 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ & Reads from the legacy common configuration structure  \\
+\hline
+0x0004 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE & Writes into the legacy device configuration structure \\
+\hline
+0x0005 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ & Reads into the legacy device configuration structure \\
+\hline
+0x0006 & VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO & Query the notification region information \\
+\hline
+0x0007 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
 \hline
 0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
 \hline
@@ -286,6 +296,8 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
 supporting multiple group types, the list of supported commands
 might differ between different group types.
 
+\input{admin-cmds-legacy-interface.tex}
+
 \devicenormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
 
 The device MUST validate \field{opcode}, \field{group_type} and
diff --git a/conformance.tex b/conformance.tex
index 01ccd69..dc00e84 100644
--- a/conformance.tex
+++ b/conformance.tex
@@ -260,6 +260,8 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
 \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
 \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
 \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
+\item Section \ref{devicenormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
+\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
 \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
 \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
 \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
-- 
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] 18+ messages in thread

* [virtio-dev] Re: [PATCH v11 3/3] admin: Add group member legacy register access commands
  2023-07-06 21:27   ` [virtio-comment] " Parav Pandit
@ 2023-07-06 22:36     ` Michael S. Tsirkin
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-07-06 22:36 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, yishaih, maorg, shahafs

On Fri, Jul 07, 2023 at 12:27:22AM +0300, Parav Pandit wrote:
> Introduce group member legacy common configuration and legacy device
> configuration access read/write commands.
> 
> Group member legacy registers access commands enable group owner driver
> software to access legacy registers on behalf of the guest virtual
> machine.
> 
> 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.
> 
> Overview:
> =========
> Above usecase requirements is solved by PCI PF group owner accessing
> its group member PCI VFs legacy registers using the administration
> commands of the group owner PCI PF.
> 
> Two types of administration commands are added which read/write PCI VF
> registers.
> 
> Software usage example:
> =======================
> 
> 1. One 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 | |
> |   | forwarder    |     |                 | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>    Config region                 |
>      access                Driver notifications
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Continue to use the virtio pci driver to bind to the
>    listed device id and use it as in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Some more comments.


> ---
> changelog:
> v10->v11:
> - replaced tab with white spaces in read structure
> - included pci fields along side other generic fields to avoid
>   indirection
> - merged pci conformance section
> - avoid using definite in starting introduction
> - replace 'all of the' with 'any of the'
> - changed drivers notification normative to indicate use of
>   NOTIFY_INFO command
> - renamed NOTIFY_QUERY to NOTIFY_INFO name
> - merged 4th patch with 3rd
> - added normative line for notify_info command
> - reworded notification region command description to be more verbose
> - merged flags and owner field to indicate end of list
> v9->v10:
> - added white space at end of line
> - addressed below comments from Cornelia
> - added missing articles
> - reworded description for notification query command
> - grammar fixes
> - addressed below comments from Michael
> - added description for member group id setting
> - reworded device and driver conformance statements
> - opcode table description updated
> - fixed label for device read command
> - length alignment restriction text added
> - data length described for read write commands
> - notification description added and refined
> - reworded text around command specific result and data field usage
> v8->v9:
> - add missing articles in notify query command
> - replaced 'this notification' with 'such a notification'
> - addressed below comments from Michael
> - dropped 'Region' from the commands
> - added 7 reserved pad bytes in config write commands
> - rewrote from 'use following structure' to 'field' has the following
>   struct..
> - dropped mentioning to follow struct virtio_admin_cmd.
> - added note about command limited to only sriov group type for now
> - rewrote the description little differently
> v7->v8:
> - remove empty line at the end of file
> - removed white space at the end
> - addressed comments from Michael add link to pci
> - renamed region to region_data
> - made region_data width to be 16 bytes to cover for 8 bytes offset
> - moved generic notification region related normative from pci to
>   generic section
> v6->v7:
> - changed administrative to administration
> - renamed admin-access.tex to admin-interface.tex
> - large rewrite ad generic admin commands instead of pci
> - added theory of operation section
> - added driver notification region query command
> v5->v6:
> - fixed previous missed abbreviation of LCC and LD
> v4->v5:
> - split from pci transport specific patch
> - split conformance to transport and generic sections
> - written the description of the command as generic with member
>   and group device terminology
> - reflected many section names to remove VF
> - rename fields from register to region
> - avoided abbreviation for legacy, device and config
> ---
>  admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
>  admin.tex                       |  14 +-
>  conformance.tex                 |   2 +
>  3 files changed, 317 insertions(+), 1 deletion(-)
>  create mode 100644 admin-cmds-legacy-interface.tex
> 
> diff --git a/admin-cmds-legacy-interface.tex b/admin-cmds-legacy-interface.tex
> new file mode 100644
> index 0000000..1ecb5af
> --- /dev/null
> +++ b/admin-cmds-legacy-interface.tex
> @@ -0,0 +1,302 @@
> +\subsubsection{Legacy Interfaces}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group
> +administration commands / Legacy Interface}
> +
> +In some systems, there is a need to support utilizing a legacy driver with
> +a device that does not directly support the legacy interface. In such scenarios,
> +a group owner device can provide the legacy interface functionality for the
> +group member devices. The driver of an owner device

the owner device


there could be more of these "the" missing, I think we need to go
over the proposal and add them more or less everywhere.
I'm yet to do this, let's get rest of wording right first.


> can then access the legacy
> +interface of a member device on behalf of the legacy member device driver.
> +
> +For example, with the SR-IOV group type, group members (VFs) can not present
> +the legacy interface in an I/O BAR in BAR0 as expected by the legacy pci driver.
> +If the legacy driver is running inside a virtual machine, the hypervisor
> +executing the virtual machine can present a virtual device with an I/O BAR in
> +BAR0. The hypervisor intercepts the legacy driver accesses to this I/O BAR and
> +forwards them to the group owner device (PF) using group administration commands.
> +
> +The following commands support such legacy interface functionality:
> +
> +\begin{enumerate}
> +\item Legacy Common Configuration Write Command
> +\item Legacy Common Configuration Read Command
> +\item Legacy Device Configuration Write Command
> +\item Legacy Device Configuration Read Command
> +\end{enumerate}
> +
> +These commands are currently only defined for the SR-IOV group type and
> +have, generally, the same effect as member device accesses through a legacy
> +interface listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} except that little endian format is assumed unconditionally.
> +
> +\paragraph{Legacy Common Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group
> +administration commands / Legacy Interface / Legacy Common Configuration Write Command}
> +
> +This command has the same effect as writing into the virtio common configuration
> +structure through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_common_cfg_wr_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_common_cfg_wr_data {
> +        u8 offset; /* Starting byte offset within the common configuration structure to write */
> +        u8 reserved[7];
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE the driver sets
> +\field{opcode} to 0x2.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. The driver sets \field{offset} which refers to the offset to write within the
> +virtio common configuration structure, and excluding the device-specific configuration.

*the value written* refers to the member not the field itself, very repetetive
and long winded and if you start saying the value there's more
repetetive noise than actual content in this sentence.

I think the way I wrote this:

The \field{group_member_id} refers to the member device to be accessed.

is cleaner and shorter.

I suggest you go back and apply such a change throughout.


> +The length of the data to write is simply the length of \field{data}.
> +
> +No length or alignment restrictions are placed on the value of the
> +\field{offset} and the length of the \field{data}, except that the resulting
> +access refers to a single field and is completely within the virtio common
> +configuration structure, excluding the device-specific configuration.
> +
> +This command has no command specific result.
> +
> +\paragraph{Legacy Common Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Common Configuration Read Command}
> +
> +This command has the same effect as reading from the virtio common configuration
> +structure through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_common_cfg_rd_data {
> +        u8 offset; /* Starting byte offset within the common configuration structure to read */
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ the driver sets
> +\field{opcode} to 0x3.
> +The driver sets \field{offset} which refers to the offset to read from the
> +virtio common configuration structure, and excluding the device-specific
> +configuration.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_common_cfg_rd_result {
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +When the command completes successfully, \field{command_specific_result}
> +is in the format of \field{struct virtio_admin_cmd_legacy_common_cfg_rd_result}
> +returned by the device. The length of the data read is simply the length of
> +\field{data}.

copy the alignment and length paragraph here too.

> +
> +\paragraph{Legacy Device Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Write Command}
> +
> +This command has the same effect as writing into the virtio device-specific
> +configuration through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_dev_reg_wr_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_dev_reg_wr_data {
> +        u8 offset; /* Starting byte offset within the device-specific configuration to write */
> +        u8 reserved[7];
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
> +\field{opcode} to 0x4.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. The driver sets \field{offset} which refers to the offset to write
> +within the virtio device-specific configuration. The length of the data to write
> +is simply the length of \field{data}.
> +
> +No length or alignment restrictions are placed on the value of the
> +\field{offset} and the length of the \field{data}, except that the resulting
> +access refers to a single field and is completely within the device-specific
> +configuration.
> +
> +This command has no command specific result.
> +
> +\paragraph{Legacy Device Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Read Command}
> +
> +This command has the same effect as reading from the virtio device-specific
> +configuration through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_dev_cfg_rd_data {
> +        u8 offset; /* Starting byte offset within the device-specific configuration to read */
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
> +\field{opcode} to 0x5.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. The driver sets \field{offset} which refers to the offset to read from
> +the virtio device-specific configuration.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_dev_reg_rd_result {
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +When the command completes successfully, \field{command_specific_result} is in
> +the format of \field{struct virtio_admin_cmd_legacy_dev_reg_rd_result}
> +returned by the device.
> +
> +The length of the data read is simply the length of \field{data}.

copy alignment here too.

> +
> +\paragraph{Legacy Driver Notification}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Driver Notifications}
> +
> +The driver of the owner device can send a driver notification to the member
> +device

operated using the legacy interface

> by executing VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE with the
> +\field{offset} matching \field{Queue Notify} and the \field{data} containing
> +the virtqueue index to be notified.
> +
> +However, as many administration commands are used for slow path configuration,

However, as VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE is also used for
slow path configuration,

> +a separate fast path mechanism for such notifications is desired.

a separate dedicated mechanism for sending such driver notifications
to the member device can be made available by the owner device

> For the
> +SR-IOV group type, the optional command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> +addresses this need by returning to the driver one or more addresses which to
> +be used

which can be used

> to send such driver notifications.


> The member driver in the hypervisor
> +uses the notification location supplied in
> +the VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO result. The member driver in the
> +hypervisor which intercepts I/O BAR writes, for the \field{Queue Notify} writes,
> +the member driver performs memory or I/O access directly in the supplied
> +notification location instead of executing slow administration command.

This is just messed up. In the introduction we explained:
	The driver of an owner device can then access the legacy
	interface of a member device on behalf of the legacy member device driver.
and let us stick to that and not invent even more terminology.
Also all the hypervisor stuff we inroduce once as
an example. I think that is enough but if not sure, give a
generic explanation then add an example.


If you are inclined to repeat yourself, the following should be enough
I feel.

	the driver of the owner device can then send driver notifications
	on behalf of the legacy member device driver.





> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO the driver sets
> +\field{opcode} to 0x6.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. This command does not use \field{command_specific_data}.
> +
> +This command is currently only defined for the SR-IOV group type.

make this a separate paragraph and drop "currently".
Also move up - maybe 1st sentence, maybe right after eneral description.


> When the
> +device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner
> +device hardwires VF BAR0 to zero in the SR-IOV Extended capability and the
> +group member device does not use PCI BAR0 in any of the Virtio PCI capabilities
> +listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Capabilities}.

either drop this second part or make it detailed. the way it's written
reader goes "so there's this extra requirement what is it" only to
then realize "wait there's no BAR0 at all".

If you want to, I suggest we work separately on a bugfix patch that
documents that capabilities MUST NOT refer to bar which is hardwired
to 0.



> +
> +\begin{lstlisting}
> +struct virtio_pci_legacy_notify_info {
> +        u8 flags;  /* 0 = end of list, 1 = owner device, 2 = member device */
> +        u8 bar;    /* BAR of the member or owner device */

the owner.


> +        u8 padding[7];
> +        le64 offset; /* Offset within bar. */
> +};
> +\end{lstlisting}
> +
> +\field{flags} value of 0x1 indicates that the notification region is of owner
> +device, value of 0x2 indicates that the notification region is of member
> +device, the value of 0 indicates that all the entries starting from that entry
> +are invalid entries in \field{entries}. All other values in \field{flags} are
> +reserved.

and now we have "notification region".
you said above "address" and sometimes. Please stick to that.
Also move the description to after both listings.
document each field not just flags. properly please,
and i hope you look up sriov and pci spec and use
official terminology from there please.

document that driver skips reserved values.

> +
> +\begin{lstlisting}
> +union virtio_admin_cmd_legacy_notify_info {
> +        u8 data[16];
> +	struct virtio_pci_legacy_notify_info pci_info;
> +};

what is doing even?  it's never mentioned anywhere.

> +
> +struct virtio_admin_cmd_legacy_notify_info_result {
> +	struct virtio_virtio_admin_cmd_legacy_notify_info entries[4];

and this?

> +};
> +\end{lstlisting}

just do:
struct virtio_admin_cmd_legacy_notify_info_result {
	struct virtio_pci_legacy_notify_info entries[4];
};

and be done with it.

> +
> +When the command completes successfully, \field{command_specific_result} is in
> +the format of \field{struct virtio_admin_cmd_legacy_notify_info_result}.


>The
> +driver can use any entry when multiple entries are supplied by the device.

The device can supply up to 4 entries each with a different
notification address. In this case, any of the entries
can be used by the driver. The order of the entries serves as
a preference hint - all other things being equal the driver
is expected to perfer utilizing the entries placed
earlier in the array to the later ones.


I notice you decided to silently ignore my suggestion to document how
are notifications performed. Repeating myself like this is despiriting
for me.
Pls re-add especially since you already document it for the
cfg_Write access method anyway.


also in a conformance section, document the effect of notification being
the same as notification through legacy interface.


> +
> +\devicenormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
> +
> +A device MUST either support all of, or none of
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the device MUST decode and
> +encode (respectively) the value of the \field{data} using the little-endian
> +format.
> +
> +The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands where the value of the
> +\field{offset} and the length of the \field{data} does not refer to a
> +single field or is not completely within the virtio common configuration
> +structure excluding the device-specific configuration.
> +
> +The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands where the value of the
> +\field{offset} and the length of the \field{data} does not refer to a
> +single field or is not completely within the device-specific configuration.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE MUST have the same effect
> +as writing into the virtio common configuration structure through the legacy
> +interface.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ MUST have the same effect as
> +reading from the virtio common configuration structure through the legacy
> +interface.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE MUST have the same effect as
> +writing into the virtio device configuration structure through the legacy
> +interface.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ MUST have the same effect as
> +reading from the virtio device configuration structure through the legacy
> +interface.
> +
> +If the device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO it MUST
> +also support all of VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
> +
> +The device MAY support VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO with entries
> +of the owner device or the member device or both of them.
> +
> +For the SR-IOV group type, when the owner device supports
> +VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner device MUST
> +hardwire VF BAR0 to zero in the SR-IOV Extended capability.
> +
> +For the SR-IOV group type, when the owner device supports
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> +commands, the owner device and the group member device SHOULD follow the rules
> +for the PCI Revision ID and Subsystem Device ID of the non-transitional devices
> +documented in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
> +
> +For the SR-IOV group type, when the owner device supports
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> +commands, the owner device SHOULD follow the rules for the PCI Device ID of the non-transitional
> +devices documented in section
> +\ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
> +
> +\drivernormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver MUST encode and
> +decode (respectively) the value of the \field{data} using the little-endian
> +format.
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands the driver SHOULD set
> +\field{offset} and the length of the \field{data} to refer to a single
> +field within the virtio common configuration structure excluding
> +the device-specific configuration.
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver SHOULD set
> +\field{offset} and the length of the \field{data} to refer to a single
> +field within the virtio device-specific configuration.
> +
> +If VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command is supported, the group member
> +driver SHOULD use the notification region to send a driver notification to the
> +device.
> +
> +When the device reports zero in \field{flags} in
> +\field{struct virtio_pci_legacy_notify_info} for the entry, the driver must
> +ignore all other fields of \field{struct virtio_pci_legacy_notify_info}.
> diff --git a/admin.tex b/admin.tex
> index b0a1a91..0803c26 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -117,7 +117,17 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline
>  0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
>  \hline
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE & Writes into the legacy common configuration structure \\
> +\hline
> +0x0003 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ & Reads from the legacy common configuration structure  \\
> +\hline
> +0x0004 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE & Writes into the legacy device configuration structure \\
> +\hline
> +0x0005 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ & Reads into the legacy device configuration structure \\
> +\hline
> +0x0006 & VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO & Query the notification region information \\
> +\hline
> +0x0007 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>  \hline
> @@ -286,6 +296,8 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  supporting multiple group types, the list of supported commands
>  might differ between different group types.
>  
> +\input{admin-cmds-legacy-interface.tex}
> +
>  \devicenormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
>  
>  The device MUST validate \field{opcode}, \field{group_type} and
> diff --git a/conformance.tex b/conformance.tex
> index 01ccd69..dc00e84 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -260,6 +260,8 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
> +\item Section \ref{devicenormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
> +\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
>  \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
>  \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
>  \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> -- 
> 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] 18+ messages in thread

* [virtio-comment] Re: [PATCH v11 3/3] admin: Add group member legacy register access commands
@ 2023-07-06 22:36     ` Michael S. Tsirkin
  0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-07-06 22:36 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, yishaih, maorg, shahafs

On Fri, Jul 07, 2023 at 12:27:22AM +0300, Parav Pandit wrote:
> Introduce group member legacy common configuration and legacy device
> configuration access read/write commands.
> 
> Group member legacy registers access commands enable group owner driver
> software to access legacy registers on behalf of the guest virtual
> machine.
> 
> 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.
> 
> Overview:
> =========
> Above usecase requirements is solved by PCI PF group owner accessing
> its group member PCI VFs legacy registers using the administration
> commands of the group owner PCI PF.
> 
> Two types of administration commands are added which read/write PCI VF
> registers.
> 
> Software usage example:
> =======================
> 
> 1. One 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 | |
> |   | forwarder    |     |                 | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>    Config region                 |
>      access                Driver notifications
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | PCI VF device A |
>   | | AQ  |-------------+---->+-------------+ |
>   | +-----+         |   |   | | legacy regs | |
>   | PCI PF device   |   |   | +-------------+ |
>   +-----------------+   |   +-----------------+
>                         |
>                         |   +----+------------+
>                         |   | PCI VF device N |
>                         +---->+-------------+ |
>                             | | legacy regs | |
>                             | +-------------+ |
>                             +-----------------+
> 
> 2. Continue to use the virtio pci driver to bind to the
>    listed device id and use it as in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>

Some more comments.


> ---
> changelog:
> v10->v11:
> - replaced tab with white spaces in read structure
> - included pci fields along side other generic fields to avoid
>   indirection
> - merged pci conformance section
> - avoid using definite in starting introduction
> - replace 'all of the' with 'any of the'
> - changed drivers notification normative to indicate use of
>   NOTIFY_INFO command
> - renamed NOTIFY_QUERY to NOTIFY_INFO name
> - merged 4th patch with 3rd
> - added normative line for notify_info command
> - reworded notification region command description to be more verbose
> - merged flags and owner field to indicate end of list
> v9->v10:
> - added white space at end of line
> - addressed below comments from Cornelia
> - added missing articles
> - reworded description for notification query command
> - grammar fixes
> - addressed below comments from Michael
> - added description for member group id setting
> - reworded device and driver conformance statements
> - opcode table description updated
> - fixed label for device read command
> - length alignment restriction text added
> - data length described for read write commands
> - notification description added and refined
> - reworded text around command specific result and data field usage
> v8->v9:
> - add missing articles in notify query command
> - replaced 'this notification' with 'such a notification'
> - addressed below comments from Michael
> - dropped 'Region' from the commands
> - added 7 reserved pad bytes in config write commands
> - rewrote from 'use following structure' to 'field' has the following
>   struct..
> - dropped mentioning to follow struct virtio_admin_cmd.
> - added note about command limited to only sriov group type for now
> - rewrote the description little differently
> v7->v8:
> - remove empty line at the end of file
> - removed white space at the end
> - addressed comments from Michael add link to pci
> - renamed region to region_data
> - made region_data width to be 16 bytes to cover for 8 bytes offset
> - moved generic notification region related normative from pci to
>   generic section
> v6->v7:
> - changed administrative to administration
> - renamed admin-access.tex to admin-interface.tex
> - large rewrite ad generic admin commands instead of pci
> - added theory of operation section
> - added driver notification region query command
> v5->v6:
> - fixed previous missed abbreviation of LCC and LD
> v4->v5:
> - split from pci transport specific patch
> - split conformance to transport and generic sections
> - written the description of the command as generic with member
>   and group device terminology
> - reflected many section names to remove VF
> - rename fields from register to region
> - avoided abbreviation for legacy, device and config
> ---
>  admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
>  admin.tex                       |  14 +-
>  conformance.tex                 |   2 +
>  3 files changed, 317 insertions(+), 1 deletion(-)
>  create mode 100644 admin-cmds-legacy-interface.tex
> 
> diff --git a/admin-cmds-legacy-interface.tex b/admin-cmds-legacy-interface.tex
> new file mode 100644
> index 0000000..1ecb5af
> --- /dev/null
> +++ b/admin-cmds-legacy-interface.tex
> @@ -0,0 +1,302 @@
> +\subsubsection{Legacy Interfaces}\label{sec:Basic Facilities of a Virtio Device / Device groups / Group
> +administration commands / Legacy Interface}
> +
> +In some systems, there is a need to support utilizing a legacy driver with
> +a device that does not directly support the legacy interface. In such scenarios,
> +a group owner device can provide the legacy interface functionality for the
> +group member devices. The driver of an owner device

the owner device


there could be more of these "the" missing, I think we need to go
over the proposal and add them more or less everywhere.
I'm yet to do this, let's get rest of wording right first.


> can then access the legacy
> +interface of a member device on behalf of the legacy member device driver.
> +
> +For example, with the SR-IOV group type, group members (VFs) can not present
> +the legacy interface in an I/O BAR in BAR0 as expected by the legacy pci driver.
> +If the legacy driver is running inside a virtual machine, the hypervisor
> +executing the virtual machine can present a virtual device with an I/O BAR in
> +BAR0. The hypervisor intercepts the legacy driver accesses to this I/O BAR and
> +forwards them to the group owner device (PF) using group administration commands.
> +
> +The following commands support such legacy interface functionality:
> +
> +\begin{enumerate}
> +\item Legacy Common Configuration Write Command
> +\item Legacy Common Configuration Read Command
> +\item Legacy Device Configuration Write Command
> +\item Legacy Device Configuration Read Command
> +\end{enumerate}
> +
> +These commands are currently only defined for the SR-IOV group type and
> +have, generally, the same effect as member device accesses through a legacy
> +interface listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout} except that little endian format is assumed unconditionally.
> +
> +\paragraph{Legacy Common Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group
> +administration commands / Legacy Interface / Legacy Common Configuration Write Command}
> +
> +This command has the same effect as writing into the virtio common configuration
> +structure through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_common_cfg_wr_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_common_cfg_wr_data {
> +        u8 offset; /* Starting byte offset within the common configuration structure to write */
> +        u8 reserved[7];
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE the driver sets
> +\field{opcode} to 0x2.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. The driver sets \field{offset} which refers to the offset to write within the
> +virtio common configuration structure, and excluding the device-specific configuration.

*the value written* refers to the member not the field itself, very repetetive
and long winded and if you start saying the value there's more
repetetive noise than actual content in this sentence.

I think the way I wrote this:

The \field{group_member_id} refers to the member device to be accessed.

is cleaner and shorter.

I suggest you go back and apply such a change throughout.


> +The length of the data to write is simply the length of \field{data}.
> +
> +No length or alignment restrictions are placed on the value of the
> +\field{offset} and the length of the \field{data}, except that the resulting
> +access refers to a single field and is completely within the virtio common
> +configuration structure, excluding the device-specific configuration.
> +
> +This command has no command specific result.
> +
> +\paragraph{Legacy Common Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Common Configuration Read Command}
> +
> +This command has the same effect as reading from the virtio common configuration
> +structure through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_common_cfg_rd_data {
> +        u8 offset; /* Starting byte offset within the common configuration structure to read */
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ the driver sets
> +\field{opcode} to 0x3.
> +The driver sets \field{offset} which refers to the offset to read from the
> +virtio common configuration structure, and excluding the device-specific
> +configuration.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_common_cfg_rd_result {
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +When the command completes successfully, \field{command_specific_result}
> +is in the format of \field{struct virtio_admin_cmd_legacy_common_cfg_rd_result}
> +returned by the device. The length of the data read is simply the length of
> +\field{data}.

copy the alignment and length paragraph here too.

> +
> +\paragraph{Legacy Device Configuration Write Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Write Command}
> +
> +This command has the same effect as writing into the virtio device-specific
> +configuration through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_dev_reg_wr_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_dev_reg_wr_data {
> +        u8 offset; /* Starting byte offset within the device-specific configuration to write */
> +        u8 reserved[7];
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
> +\field{opcode} to 0x4.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. The driver sets \field{offset} which refers to the offset to write
> +within the virtio device-specific configuration. The length of the data to write
> +is simply the length of \field{data}.
> +
> +No length or alignment restrictions are placed on the value of the
> +\field{offset} and the length of the \field{data}, except that the resulting
> +access refers to a single field and is completely within the device-specific
> +configuration.
> +
> +This command has no command specific result.
> +
> +\paragraph{Legacy Device Configuration Read Command}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Device Configuration Read Command}
> +
> +This command has the same effect as reading from the virtio device-specific
> +configuration through the legacy interface. The \field{command_specific_data} is in
> +the format \field{struct virtio_admin_cmd_legacy_common_cfg_rd_data} describing
> +the access to be performed.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_dev_cfg_rd_data {
> +        u8 offset; /* Starting byte offset within the device-specific configuration to read */
> +};
> +\end{lstlisting}
> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE the driver sets
> +\field{opcode} to 0x5.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. The driver sets \field{offset} which refers to the offset to read from
> +the virtio device-specific configuration.
> +
> +\begin{lstlisting}
> +struct virtio_admin_cmd_legacy_dev_reg_rd_result {
> +        u8 data[];
> +};
> +\end{lstlisting}
> +
> +When the command completes successfully, \field{command_specific_result} is in
> +the format of \field{struct virtio_admin_cmd_legacy_dev_reg_rd_result}
> +returned by the device.
> +
> +The length of the data read is simply the length of \field{data}.

copy alignment here too.

> +
> +\paragraph{Legacy Driver Notification}\label{par:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface / Legacy Driver Notifications}
> +
> +The driver of the owner device can send a driver notification to the member
> +device

operated using the legacy interface

> by executing VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE with the
> +\field{offset} matching \field{Queue Notify} and the \field{data} containing
> +the virtqueue index to be notified.
> +
> +However, as many administration commands are used for slow path configuration,

However, as VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE is also used for
slow path configuration,

> +a separate fast path mechanism for such notifications is desired.

a separate dedicated mechanism for sending such driver notifications
to the member device can be made available by the owner device

> For the
> +SR-IOV group type, the optional command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> +addresses this need by returning to the driver one or more addresses which to
> +be used

which can be used

> to send such driver notifications.


> The member driver in the hypervisor
> +uses the notification location supplied in
> +the VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO result. The member driver in the
> +hypervisor which intercepts I/O BAR writes, for the \field{Queue Notify} writes,
> +the member driver performs memory or I/O access directly in the supplied
> +notification location instead of executing slow administration command.

This is just messed up. In the introduction we explained:
	The driver of an owner device can then access the legacy
	interface of a member device on behalf of the legacy member device driver.
and let us stick to that and not invent even more terminology.
Also all the hypervisor stuff we inroduce once as
an example. I think that is enough but if not sure, give a
generic explanation then add an example.


If you are inclined to repeat yourself, the following should be enough
I feel.

	the driver of the owner device can then send driver notifications
	on behalf of the legacy member device driver.





> +
> +For the command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO the driver sets
> +\field{opcode} to 0x6.
> +The driver sets \field{group_member_id} which refers to the member device to be
> +accessed. This command does not use \field{command_specific_data}.
> +
> +This command is currently only defined for the SR-IOV group type.

make this a separate paragraph and drop "currently".
Also move up - maybe 1st sentence, maybe right after eneral description.


> When the
> +device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner
> +device hardwires VF BAR0 to zero in the SR-IOV Extended capability and the
> +group member device does not use PCI BAR0 in any of the Virtio PCI capabilities
> +listed in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Capabilities}.

either drop this second part or make it detailed. the way it's written
reader goes "so there's this extra requirement what is it" only to
then realize "wait there's no BAR0 at all".

If you want to, I suggest we work separately on a bugfix patch that
documents that capabilities MUST NOT refer to bar which is hardwired
to 0.



> +
> +\begin{lstlisting}
> +struct virtio_pci_legacy_notify_info {
> +        u8 flags;  /* 0 = end of list, 1 = owner device, 2 = member device */
> +        u8 bar;    /* BAR of the member or owner device */

the owner.


> +        u8 padding[7];
> +        le64 offset; /* Offset within bar. */
> +};
> +\end{lstlisting}
> +
> +\field{flags} value of 0x1 indicates that the notification region is of owner
> +device, value of 0x2 indicates that the notification region is of member
> +device, the value of 0 indicates that all the entries starting from that entry
> +are invalid entries in \field{entries}. All other values in \field{flags} are
> +reserved.

and now we have "notification region".
you said above "address" and sometimes. Please stick to that.
Also move the description to after both listings.
document each field not just flags. properly please,
and i hope you look up sriov and pci spec and use
official terminology from there please.

document that driver skips reserved values.

> +
> +\begin{lstlisting}
> +union virtio_admin_cmd_legacy_notify_info {
> +        u8 data[16];
> +	struct virtio_pci_legacy_notify_info pci_info;
> +};

what is doing even?  it's never mentioned anywhere.

> +
> +struct virtio_admin_cmd_legacy_notify_info_result {
> +	struct virtio_virtio_admin_cmd_legacy_notify_info entries[4];

and this?

> +};
> +\end{lstlisting}

just do:
struct virtio_admin_cmd_legacy_notify_info_result {
	struct virtio_pci_legacy_notify_info entries[4];
};

and be done with it.

> +
> +When the command completes successfully, \field{command_specific_result} is in
> +the format of \field{struct virtio_admin_cmd_legacy_notify_info_result}.


>The
> +driver can use any entry when multiple entries are supplied by the device.

The device can supply up to 4 entries each with a different
notification address. In this case, any of the entries
can be used by the driver. The order of the entries serves as
a preference hint - all other things being equal the driver
is expected to perfer utilizing the entries placed
earlier in the array to the later ones.


I notice you decided to silently ignore my suggestion to document how
are notifications performed. Repeating myself like this is despiriting
for me.
Pls re-add especially since you already document it for the
cfg_Write access method anyway.


also in a conformance section, document the effect of notification being
the same as notification through legacy interface.


> +
> +\devicenormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
> +
> +A device MUST either support all of, or none of
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the device MUST decode and
> +encode (respectively) the value of the \field{data} using the little-endian
> +format.
> +
> +The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands where the value of the
> +\field{offset} and the length of the \field{data} does not refer to a
> +single field or is not completely within the virtio common configuration
> +structure excluding the device-specific configuration.
> +
> +The device MUST fail VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands where the value of the
> +\field{offset} and the length of the \field{data} does not refer to a
> +single field or is not completely within the device-specific configuration.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE MUST have the same effect
> +as writing into the virtio common configuration structure through the legacy
> +interface.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ MUST have the same effect as
> +reading from the virtio common configuration structure through the legacy
> +interface.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE MUST have the same effect as
> +writing into the virtio device configuration structure through the legacy
> +interface.
> +
> +The command VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ MUST have the same effect as
> +reading from the virtio device configuration structure through the legacy
> +interface.
> +
> +If the device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO it MUST
> +also support all of VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands.
> +
> +The device MAY support VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO with entries
> +of the owner device or the member device or both of them.
> +
> +For the SR-IOV group type, when the owner device supports
> +VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command, the group owner device MUST
> +hardwire VF BAR0 to zero in the SR-IOV Extended capability.
> +
> +For the SR-IOV group type, when the owner device supports
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> +commands, the owner device and the group member device SHOULD follow the rules
> +for the PCI Revision ID and Subsystem Device ID of the non-transitional devices
> +documented in section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
> +
> +For the SR-IOV group type, when the owner device supports
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE, VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ,
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> +commands, the owner device SHOULD follow the rules for the PCI Device ID of the non-transitional
> +devices documented in section
> +\ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery}.
> +
> +\drivernormative{\paragraph}{Legacy Interface}{Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver MUST encode and
> +decode (respectively) the value of the \field{data} using the little-endian
> +format.
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ commands the driver SHOULD set
> +\field{offset} and the length of the \field{data} to refer to a single
> +field within the virtio common configuration structure excluding
> +the device-specific configuration.
> +
> +For VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE and
> +VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ commands, the driver SHOULD set
> +\field{offset} and the length of the \field{data} to refer to a single
> +field within the virtio device-specific configuration.
> +
> +If VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command is supported, the group member
> +driver SHOULD use the notification region to send a driver notification to the
> +device.
> +
> +When the device reports zero in \field{flags} in
> +\field{struct virtio_pci_legacy_notify_info} for the entry, the driver must
> +ignore all other fields of \field{struct virtio_pci_legacy_notify_info}.
> diff --git a/admin.tex b/admin.tex
> index b0a1a91..0803c26 100644
> --- a/admin.tex
> +++ b/admin.tex
> @@ -117,7 +117,17 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  \hline
>  0x0001 & VIRTIO_ADMIN_CMD_LIST_USE & Provides to device list of commands used for this group type \\
>  \hline
> -0x0002 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
> +0x0002 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE & Writes into the legacy common configuration structure \\
> +\hline
> +0x0003 & VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_READ & Reads from the legacy common configuration structure  \\
> +\hline
> +0x0004 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_WRITE & Writes into the legacy device configuration structure \\
> +\hline
> +0x0005 & VIRTIO_ADMIN_CMD_LEGACY_DEV_CFG_READ & Reads into the legacy device configuration structure \\
> +\hline
> +0x0006 & VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO & Query the notification region information \\
> +\hline
> +0x0007 - 0x7FFF & - & Commands using \field{struct virtio_admin_cmd}    \\
>  \hline
>  0x8000 - 0xFFFF & - & Reserved for future commands (possibly using a different structure)    \\
>  \hline
> @@ -286,6 +296,8 @@ \subsection{Group administration commands}\label{sec:Basic Facilities of a Virti
>  supporting multiple group types, the list of supported commands
>  might differ between different group types.
>  
> +\input{admin-cmds-legacy-interface.tex}
> +
>  \devicenormative{\subsubsection}{Group administration commands}{Basic Facilities of a Virtio Device / Device groups / Group administration commands}
>  
>  The device MUST validate \field{opcode}, \field{group_type} and
> diff --git a/conformance.tex b/conformance.tex
> index 01ccd69..dc00e84 100644
> --- a/conformance.tex
> +++ b/conformance.tex
> @@ -260,6 +260,8 @@ \section{Conformance Targets}\label{sec:Conformance / Conformance Targets}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Layout}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Legacy Interfaces: A Note on Virtqueue Endianness}
>  \item Section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Message Framing / Legacy Interface: Message Framing}
> +\item Section \ref{devicenormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
> +\item Section \ref{drivernormative:Basic Facilities of a Virtio Device / Device groups / Group administration commands / Legacy Interface}
>  \item Section \ref{sec:General Initialization And Device Operation / Device Initialization / Legacy Interface: Device Initialization}
>  \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Discovery / Legacy Interfaces: A Note on PCI Device Discovery}
>  \item Section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Legacy Interfaces: A Note on PCI Device Layout}
> -- 
> 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] 18+ messages in thread

* [virtio-dev] Re: [PATCH v11 0/3] admin: Access legacy registers using admin commands
  2023-07-06 21:27 ` [virtio-comment] " Parav Pandit
@ 2023-07-06 22:36   ` Michael S. Tsirkin
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-07-06 22:36 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, yishaih, maorg, shahafs

On Fri, Jul 07, 2023 at 12:27:19AM +0300, Parav Pandit wrote:
> This short series introduces legacy registers access commands for the owner
> group member access the legacy registers of the member VFs.
> This short series introduces legacy region access commands by the group owner
> device for its member devices.
> Currently it is applicable to the PCI PF and VF devices. 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.

corneli want to apply 1,2 as editorial?

> Patch summary:
> --------------
> patch-1 split rows of admin opcode tables by a line
> patch-2 fix section numbering
> patch-3 add legacy region access commands
> 
> It uses the newly introduced administration command facility with 4 new
> commands and a new optional command to query the legacy notification region.
> 
> 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.
> 
> Overview:
> ---------
> Above usecase requirements is solved by PCI PF group owner accessing
> its group member PCI VFs legacy registers using an admin virtqueue of
> the group owner PCI PF.
> 
> Two new admin virtqueue commands are added which read/write PCI VF
> registers.
> 
> Software usage example:
> -----------------------
> One 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 | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>    Legacy region            Driver notification
>     access                       |
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | 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 in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Alternatives considered:
> ========================
> 1. Exposing BAR0 as MMIO BAR that follows legacy registers template
> Pros:
> a. Kind of works with legacy drivers as some of them have used API
>    which is agnostic to MMIO vs IOBAR.
> b. Does not require hypervisor intervantion
> Cons:
> a. Device reset is extremely hard to implement in device at scale as
>    driver does not wait for device reset completion
> b. Device register width related problems persist that hypervisor if
>    wishes, it cannot be fixed.
> 
> 2. Accessing VF registers by tunneling it through new legacy PCI capability
> Pros:
> a. Self contained, but cannot work with future PCI SIOV devices
> Cons:
> a. Equally slow as AQ access
> b. Still requires new capability for notification access
> c. Requires hardware to build low level registers access which is not worth
>    for long term future
> 
> 3. Accessing VF notification region using new PF BAR
> Cons:
> a. Requires hardware to build new PCI steering logic per PF to forward
>    notification from the PF to VF, requires double the amount of logic
>    compared to today
> b. Requires very large additional PF BAR whose size must be max_Vfs * BAR size.
> 
> 4. Trapping CVQ, configuration region, LEGACY_HDR
> Cons:
> a. This does not fullfil the very basic requirement to not trap the
>    1.x objects (configuration registers, vqs)
> b. Requires feature negotiations mediation in hypervisor software
> c. Requires constant device type specific knowledge in hypervisor driver
>    (Does not scale for 30+ device types)
> 
> 4. F_LEACY_HDR, F_WRITE_MAC
> Cons:
> a. Requires device support to have read/write mac address which is
>    hard to implement on every member device.
> b. such functionality is duplicate of existing cvq per device.
> c. config space is only for the initialization specific purpose.
> d. Requires mediation of 1.x objects, which is not good design.
> e. Solves only for the net device.
> Pros:
> a. May work for nested env
> 
> conclusion for picking AQ approach:
> ==================================
> 1. Overall AQ based access is simpler to implement with combination of
>    best from software and device so that legacy registers do not get baked
>    in the device hardware
> 2. AQ allows hypervisor software to intercept legacy registers and make
>    corrections if needed
> 3. Provides trade-off between performance, device complexity vs spec,
>    while still maintaining passthrough mode for the VFs with minimal
>    hypervisor intercepts only for legacy registers access
> 4. AQ mechanism is designed for accessing other member devices registers
>    as noted in AQ submission, it utilizes the existing infrastructure over
>    other alternatives.
> 5. Uses existing driver notification region similar to legacy notification
>    saves hardware resources
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v10->v11:
> - replaced tab with white spaces in read structure
> - included pci fields along side other generic fields to avoid
>   indirection
> - merged pci conformance section
> - avoid using definite in starting introduction
> - replace 'all of the' with 'any of the'
> - changed drivers notification normative to indicate use of
>   NOTIFY_INFO command
> - renamed NOTIFY_QUERY to NOTIFY_INFO name
> - merged 4th patch with 3rd
> - added normative line for notify_info command
> - reworded notification region command description to be more verbose
> - merged flags and owner field to indicate end of list
> v9->v10:
> - added white space at end of line
> - addressed below comments from Cornelia
> - fixed errors related to article
> - hardwire to hardwires
> - replaced various to all
> - added hardwire to zero
> - fixed requirements for administration virtqueue section
> - added missing articles
> - reworded description for notification query command
> - grammar fixes
> - addressed below comments from Michael
> - added description for member group id setting
> - reworded device and driver conformance statements
> - opcode table description updated
> - fixed label for device read command
> - length alignment restriction text added
> - data length described for read write commands
> - notification description added and refined
> - reworded text around command specific result and data field usage
> v8->v9:
> - add missing articles in notify query command
> - replaced 'this notification' with 'such a notification'
> - addressed below comments from Michael
> - dropped 'Region' from the commands
> - added 7 reserved pad bytes in config write commands
> - rewrote from 'use following structure' to 'field' has the following
>   struct..
> - dropped mentioning to follow struct virtio_admin_cmd.
> - added note about command limited to only sriov group type for now
> - rewrote the description little differently
> v7->v8:
> - remove empty line at the end of file
> - removed white space at the end
> - addressed comments from Michael add link to pci
> - renamed region to region_data
> - made region_data width to be 16 bytes to cover for 8 bytes offset
> - moved generic notification region related normative from pci to
>   generic section
> - addressed comments from Michael
> - made bar offset 64-bit
> - prefix legacy specific structure with _legacy
> - moved generic normative from pci to generic section
> - added link to virtio pci capabilities when referring to bar 0
> - remove 'should' from generic description
> v6->v7:
> - addressed several comments from Michael
> - use AQ command to query legacy notify region, dropped pci capability
>   modifications
> - moved most part of the text to the generic admin command section
> - replace administrative to administration
> - replace admin vq citation to admin commands
> - added normatives for device and driver side
> - made BAR0 to be not used at all when supporting legacy interface
> - added normative around BAR0 and SR-IOV extended capability
> - grammar corrections
> v5->v6:
> - fixed previous missed abbreviation of LCC and LD
> - added text for the PCI capability for the group member device
> v4->v5:
> - split pci transport and generic command section to new patch
> - removed multiple references to the VF
> - written the description of the command as generic with member
>   and group device terminology
> - reflected many section names to remove VF
> - split from pci transport specific patch
> - split conformance to transport and generic sections
> - written the description of the command as generic with member
>   and group device terminology
> - reflected many section names to remove VF
> - rename fields from register to region
> - avoided abbreviation for legacy, device and config
> v3->v4:
> - moved noted to the conformance section details in next patch
> - removed queue notify address query AQ command on Michael's suggestion,
>   though it is fine. Instead replaced with extending virtio_pci_notify_cap
>   to indicate that legacy queue notifications can be done on the
>   notification location
> - fixed spelling errors
> - replaced administrative virtqueue to administration virtqueue
> - moved legacy interface normative references to legacy conformance
>   section
> v2->v3:
> - added new patch to split raws of admin vq opcode table
> - adddressed Jason and Michael's comment to split single register
>   access command to common config and device specific commands.
> - dropped the suggetion to introduce enable/disable command as
>   admin command cap bit already covers it.
> - added other alternative design considered and discussed in detail in v0, v1 and v2
> v1->v2:
> - addressed comments from Michael
> - added theory of operation
> - grammar corrections
> - removed group fields description from individual commands as
>   it is already present in generic section
> - added endianness normative for legacy device registers region
> - renamed the file to drop vf and add legacy prefix
> - added overview in commit log
> - renamed subsection to reflect command
> 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 (3):
>   admin: Split opcode table rows with a line
>   admin: Fix section numbering
>   admin: Add group member legacy register access commands
> 
>  admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
>  admin.tex                       |  24 ++-
>  conformance.tex                 |   2 +
>  3 files changed, 323 insertions(+), 5 deletions(-)
>  create mode 100644 admin-cmds-legacy-interface.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] 18+ messages in thread

* [virtio-comment] Re: [PATCH v11 0/3] admin: Access legacy registers using admin commands
@ 2023-07-06 22:36   ` Michael S. Tsirkin
  0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-07-06 22:36 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, yishaih, maorg, shahafs

On Fri, Jul 07, 2023 at 12:27:19AM +0300, Parav Pandit wrote:
> This short series introduces legacy registers access commands for the owner
> group member access the legacy registers of the member VFs.
> This short series introduces legacy region access commands by the group owner
> device for its member devices.
> Currently it is applicable to the PCI PF and VF devices. 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.

corneli want to apply 1,2 as editorial?

> Patch summary:
> --------------
> patch-1 split rows of admin opcode tables by a line
> patch-2 fix section numbering
> patch-3 add legacy region access commands
> 
> It uses the newly introduced administration command facility with 4 new
> commands and a new optional command to query the legacy notification region.
> 
> 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.
> 
> Overview:
> ---------
> Above usecase requirements is solved by PCI PF group owner accessing
> its group member PCI VFs legacy registers using an admin virtqueue of
> the group owner PCI PF.
> 
> Two new admin virtqueue commands are added which read/write PCI VF
> registers.
> 
> Software usage example:
> -----------------------
> One 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 | |
> |   +--------------+     +-----------------+ |
> |                                            |
> +------+-------------------------+-----------+
>        |                         |
>    Legacy region            Driver notification
>     access                       |
>        |                         |
>   +----+------------+       +----+------------+
>   | +-----+         |       | 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 in the host.
> 
> 3. Use it in a light weight hypervisor to run bare-metal OS.
> 
> Please review.
> 
> Alternatives considered:
> ========================
> 1. Exposing BAR0 as MMIO BAR that follows legacy registers template
> Pros:
> a. Kind of works with legacy drivers as some of them have used API
>    which is agnostic to MMIO vs IOBAR.
> b. Does not require hypervisor intervantion
> Cons:
> a. Device reset is extremely hard to implement in device at scale as
>    driver does not wait for device reset completion
> b. Device register width related problems persist that hypervisor if
>    wishes, it cannot be fixed.
> 
> 2. Accessing VF registers by tunneling it through new legacy PCI capability
> Pros:
> a. Self contained, but cannot work with future PCI SIOV devices
> Cons:
> a. Equally slow as AQ access
> b. Still requires new capability for notification access
> c. Requires hardware to build low level registers access which is not worth
>    for long term future
> 
> 3. Accessing VF notification region using new PF BAR
> Cons:
> a. Requires hardware to build new PCI steering logic per PF to forward
>    notification from the PF to VF, requires double the amount of logic
>    compared to today
> b. Requires very large additional PF BAR whose size must be max_Vfs * BAR size.
> 
> 4. Trapping CVQ, configuration region, LEGACY_HDR
> Cons:
> a. This does not fullfil the very basic requirement to not trap the
>    1.x objects (configuration registers, vqs)
> b. Requires feature negotiations mediation in hypervisor software
> c. Requires constant device type specific knowledge in hypervisor driver
>    (Does not scale for 30+ device types)
> 
> 4. F_LEACY_HDR, F_WRITE_MAC
> Cons:
> a. Requires device support to have read/write mac address which is
>    hard to implement on every member device.
> b. such functionality is duplicate of existing cvq per device.
> c. config space is only for the initialization specific purpose.
> d. Requires mediation of 1.x objects, which is not good design.
> e. Solves only for the net device.
> Pros:
> a. May work for nested env
> 
> conclusion for picking AQ approach:
> ==================================
> 1. Overall AQ based access is simpler to implement with combination of
>    best from software and device so that legacy registers do not get baked
>    in the device hardware
> 2. AQ allows hypervisor software to intercept legacy registers and make
>    corrections if needed
> 3. Provides trade-off between performance, device complexity vs spec,
>    while still maintaining passthrough mode for the VFs with minimal
>    hypervisor intercepts only for legacy registers access
> 4. AQ mechanism is designed for accessing other member devices registers
>    as noted in AQ submission, it utilizes the existing infrastructure over
>    other alternatives.
> 5. Uses existing driver notification region similar to legacy notification
>    saves hardware resources
> 
> Fixes: https://github.com/oasis-tcs/virtio-spec/issues/167
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> 
> ---
> changelog:
> v10->v11:
> - replaced tab with white spaces in read structure
> - included pci fields along side other generic fields to avoid
>   indirection
> - merged pci conformance section
> - avoid using definite in starting introduction
> - replace 'all of the' with 'any of the'
> - changed drivers notification normative to indicate use of
>   NOTIFY_INFO command
> - renamed NOTIFY_QUERY to NOTIFY_INFO name
> - merged 4th patch with 3rd
> - added normative line for notify_info command
> - reworded notification region command description to be more verbose
> - merged flags and owner field to indicate end of list
> v9->v10:
> - added white space at end of line
> - addressed below comments from Cornelia
> - fixed errors related to article
> - hardwire to hardwires
> - replaced various to all
> - added hardwire to zero
> - fixed requirements for administration virtqueue section
> - added missing articles
> - reworded description for notification query command
> - grammar fixes
> - addressed below comments from Michael
> - added description for member group id setting
> - reworded device and driver conformance statements
> - opcode table description updated
> - fixed label for device read command
> - length alignment restriction text added
> - data length described for read write commands
> - notification description added and refined
> - reworded text around command specific result and data field usage
> v8->v9:
> - add missing articles in notify query command
> - replaced 'this notification' with 'such a notification'
> - addressed below comments from Michael
> - dropped 'Region' from the commands
> - added 7 reserved pad bytes in config write commands
> - rewrote from 'use following structure' to 'field' has the following
>   struct..
> - dropped mentioning to follow struct virtio_admin_cmd.
> - added note about command limited to only sriov group type for now
> - rewrote the description little differently
> v7->v8:
> - remove empty line at the end of file
> - removed white space at the end
> - addressed comments from Michael add link to pci
> - renamed region to region_data
> - made region_data width to be 16 bytes to cover for 8 bytes offset
> - moved generic notification region related normative from pci to
>   generic section
> - addressed comments from Michael
> - made bar offset 64-bit
> - prefix legacy specific structure with _legacy
> - moved generic normative from pci to generic section
> - added link to virtio pci capabilities when referring to bar 0
> - remove 'should' from generic description
> v6->v7:
> - addressed several comments from Michael
> - use AQ command to query legacy notify region, dropped pci capability
>   modifications
> - moved most part of the text to the generic admin command section
> - replace administrative to administration
> - replace admin vq citation to admin commands
> - added normatives for device and driver side
> - made BAR0 to be not used at all when supporting legacy interface
> - added normative around BAR0 and SR-IOV extended capability
> - grammar corrections
> v5->v6:
> - fixed previous missed abbreviation of LCC and LD
> - added text for the PCI capability for the group member device
> v4->v5:
> - split pci transport and generic command section to new patch
> - removed multiple references to the VF
> - written the description of the command as generic with member
>   and group device terminology
> - reflected many section names to remove VF
> - split from pci transport specific patch
> - split conformance to transport and generic sections
> - written the description of the command as generic with member
>   and group device terminology
> - reflected many section names to remove VF
> - rename fields from register to region
> - avoided abbreviation for legacy, device and config
> v3->v4:
> - moved noted to the conformance section details in next patch
> - removed queue notify address query AQ command on Michael's suggestion,
>   though it is fine. Instead replaced with extending virtio_pci_notify_cap
>   to indicate that legacy queue notifications can be done on the
>   notification location
> - fixed spelling errors
> - replaced administrative virtqueue to administration virtqueue
> - moved legacy interface normative references to legacy conformance
>   section
> v2->v3:
> - added new patch to split raws of admin vq opcode table
> - adddressed Jason and Michael's comment to split single register
>   access command to common config and device specific commands.
> - dropped the suggetion to introduce enable/disable command as
>   admin command cap bit already covers it.
> - added other alternative design considered and discussed in detail in v0, v1 and v2
> v1->v2:
> - addressed comments from Michael
> - added theory of operation
> - grammar corrections
> - removed group fields description from individual commands as
>   it is already present in generic section
> - added endianness normative for legacy device registers region
> - renamed the file to drop vf and add legacy prefix
> - added overview in commit log
> - renamed subsection to reflect command
> 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 (3):
>   admin: Split opcode table rows with a line
>   admin: Fix section numbering
>   admin: Add group member legacy register access commands
> 
>  admin-cmds-legacy-interface.tex | 302 ++++++++++++++++++++++++++++++++
>  admin.tex                       |  24 ++-
>  conformance.tex                 |   2 +
>  3 files changed, 323 insertions(+), 5 deletions(-)
>  create mode 100644 admin-cmds-legacy-interface.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] 18+ messages in thread

* [virtio-dev] RE: [PATCH v11 3/3] admin: Add group member legacy register access commands
  2023-07-06 22:36     ` [virtio-comment] " Michael S. Tsirkin
@ 2023-07-07  3:54       ` Parav Pandit
  -1 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-07  3:54 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, Yishai Hadas, Maor Gottlieb, Shahaf Shuler


> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, July 6, 2023 6:36 PM

[..]

> I notice you decided to silently ignore my suggestion to document how are
> notifications performed. Repeating myself like this is despiriting for me.
I am sorry if it appeared that way,
But no, I didn’t silently ignored.

I added the description as best I could find it, but you commented about it being messy with mixing up the terminology.
I rewrote it in v12, it looks better now. Please check.

> Pls re-add especially since you already document it for the cfg_Write access
> method anyway.
> 
Added without citation to hypervisor etc.

> 
> also in a conformance section, document the effect of notification being the
> same as notification through legacy interface.

PCI specific things were copied from the current spec reference in the notification capability section.
All the changes you suggested are done.
Captured in the change log of v12.

Thanks a lot.
Since many parts are rewritten as you suggested in the thread, 
I prefer to add your Signed-off to in v13 if we need roll it or when it is merged, please apply if you find it appropriate.
It is at least important to me to add it yours Sign-off.

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

* [virtio-comment] RE: [PATCH v11 3/3] admin: Add group member legacy register access commands
@ 2023-07-07  3:54       ` Parav Pandit
  0 siblings, 0 replies; 18+ messages in thread
From: Parav Pandit @ 2023-07-07  3:54 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, Yishai Hadas, Maor Gottlieb, Shahaf Shuler


> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Thursday, July 6, 2023 6:36 PM

[..]

> I notice you decided to silently ignore my suggestion to document how are
> notifications performed. Repeating myself like this is despiriting for me.
I am sorry if it appeared that way,
But no, I didn’t silently ignored.

I added the description as best I could find it, but you commented about it being messy with mixing up the terminology.
I rewrote it in v12, it looks better now. Please check.

> Pls re-add especially since you already document it for the cfg_Write access
> method anyway.
> 
Added without citation to hypervisor etc.

> 
> also in a conformance section, document the effect of notification being the
> same as notification through legacy interface.

PCI specific things were copied from the current spec reference in the notification capability section.
All the changes you suggested are done.
Captured in the change log of v12.

Thanks a lot.
Since many parts are rewritten as you suggested in the thread, 
I prefer to add your Signed-off to in v13 if we need roll it or when it is merged, please apply if you find it appropriate.
It is at least important to me to add it yours Sign-off.

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

* [virtio-dev] Re: [PATCH v11 3/3] admin: Add group member legacy register access commands
  2023-07-07  3:54       ` [virtio-comment] " Parav Pandit
@ 2023-07-07  8:29         ` Michael S. Tsirkin
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-07-07  8:29 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, Yishai Hadas, Maor Gottlieb, Shahaf Shuler

On Fri, Jul 07, 2023 at 03:54:31AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, July 6, 2023 6:36 PM
> 
> [..]
> 
> > I notice you decided to silently ignore my suggestion to document how are
> > notifications performed. Repeating myself like this is despiriting for me.
> I am sorry if it appeared that way,
> But no, I didn’t silently ignored.
> 
> I added the description as best I could find it, but you commented about it being messy with mixing up the terminology.
> I rewrote it in v12, it looks better now. Please check.
> 
> > Pls re-add especially since you already document it for the cfg_Write access
> > method anyway.
> > 
> Added without citation to hypervisor etc.
> 
> > 
> > also in a conformance section, document the effect of notification being the
> > same as notification through legacy interface.
> 
> PCI specific things were copied from the current spec reference in the notification capability section.
> All the changes you suggested are done.
> Captured in the change log of v12.
> 
> Thanks a lot.
> Since many parts are rewritten as you suggested in the thread, 
> I prefer to add your Signed-off to in v13 if we need roll it or when it is merged, please apply if you find it appropriate.
> It is at least important to me to add it yours Sign-off.

If you like sure.

-- 
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] 18+ messages in thread

* [virtio-comment] Re: [PATCH v11 3/3] admin: Add group member legacy register access commands
@ 2023-07-07  8:29         ` Michael S. Tsirkin
  0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-07-07  8:29 UTC (permalink / raw)
  To: Parav Pandit
  Cc: virtio-comment, cohuck, david.edmondson, virtio-dev, sburla,
	jasowang, Yishai Hadas, Maor Gottlieb, Shahaf Shuler

On Fri, Jul 07, 2023 at 03:54:31AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, July 6, 2023 6:36 PM
> 
> [..]
> 
> > I notice you decided to silently ignore my suggestion to document how are
> > notifications performed. Repeating myself like this is despiriting for me.
> I am sorry if it appeared that way,
> But no, I didn’t silently ignored.
> 
> I added the description as best I could find it, but you commented about it being messy with mixing up the terminology.
> I rewrote it in v12, it looks better now. Please check.
> 
> > Pls re-add especially since you already document it for the cfg_Write access
> > method anyway.
> > 
> Added without citation to hypervisor etc.
> 
> > 
> > also in a conformance section, document the effect of notification being the
> > same as notification through legacy interface.
> 
> PCI specific things were copied from the current spec reference in the notification capability section.
> All the changes you suggested are done.
> Captured in the change log of v12.
> 
> Thanks a lot.
> Since many parts are rewritten as you suggested in the thread, 
> I prefer to add your Signed-off to in v13 if we need roll it or when it is merged, please apply if you find it appropriate.
> It is at least important to me to add it yours Sign-off.

If you like sure.

-- 
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] 18+ messages in thread

* [virtio-dev] Re: [PATCH v11 0/3] admin: Access legacy registers using admin commands
  2023-07-06 22:36   ` [virtio-comment] " Michael S. Tsirkin
@ 2023-07-07 11:37     ` Cornelia Huck
  -1 siblings, 0 replies; 18+ messages in thread
From: Cornelia Huck @ 2023-07-07 11:37 UTC (permalink / raw)
  To: Michael S. Tsirkin, Parav Pandit
  Cc: virtio-comment, david.edmondson, virtio-dev, sburla, jasowang,
	yishaih, maorg, shahafs

On Thu, Jul 06 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jul 07, 2023 at 12:27:19AM +0300, Parav Pandit wrote:
>> This short series introduces legacy registers access commands for the owner
>> group member access the legacy registers of the member VFs.
>> This short series introduces legacy region access commands by the group owner
>> device for its member devices.
>> Currently it is applicable to the PCI PF and VF devices. 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.
>
> corneli want to apply 1,2 as editorial?

I guess that makes sense to get them out of the way.


---------------------------------------------------------------------
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] 18+ messages in thread

* [virtio-comment] Re: [PATCH v11 0/3] admin: Access legacy registers using admin commands
@ 2023-07-07 11:37     ` Cornelia Huck
  0 siblings, 0 replies; 18+ messages in thread
From: Cornelia Huck @ 2023-07-07 11:37 UTC (permalink / raw)
  To: Michael S. Tsirkin, Parav Pandit
  Cc: virtio-comment, david.edmondson, virtio-dev, sburla, jasowang,
	yishaih, maorg, shahafs

On Thu, Jul 06 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Jul 07, 2023 at 12:27:19AM +0300, Parav Pandit wrote:
>> This short series introduces legacy registers access commands for the owner
>> group member access the legacy registers of the member VFs.
>> This short series introduces legacy region access commands by the group owner
>> device for its member devices.
>> Currently it is applicable to the PCI PF and VF devices. 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.
>
> corneli want to apply 1,2 as editorial?

I guess that makes sense to get them out of the way.


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] 18+ messages in thread

end of thread, other threads:[~2023-07-07 11:37 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-06 21:27 [virtio-dev] [PATCH v11 0/3] admin: Access legacy registers using admin commands Parav Pandit
2023-07-06 21:27 ` [virtio-comment] " Parav Pandit
2023-07-06 21:27 ` [virtio-dev] [PATCH v11 1/3] admin: Split opcode table rows with a line Parav Pandit
2023-07-06 21:27   ` [virtio-comment] " Parav Pandit
2023-07-06 21:27 ` [virtio-dev] [PATCH v11 2/3] admin: Fix section numbering Parav Pandit
2023-07-06 21:27   ` [virtio-comment] " Parav Pandit
2023-07-06 21:27 ` [virtio-dev] [PATCH v11 3/3] admin: Add group member legacy register access commands Parav Pandit
2023-07-06 21:27   ` [virtio-comment] " Parav Pandit
2023-07-06 22:36   ` [virtio-dev] " Michael S. Tsirkin
2023-07-06 22:36     ` [virtio-comment] " Michael S. Tsirkin
2023-07-07  3:54     ` [virtio-dev] " Parav Pandit
2023-07-07  3:54       ` [virtio-comment] " Parav Pandit
2023-07-07  8:29       ` [virtio-dev] " Michael S. Tsirkin
2023-07-07  8:29         ` [virtio-comment] " Michael S. Tsirkin
2023-07-06 22:36 ` [virtio-dev] Re: [PATCH v11 0/3] admin: Access legacy registers using admin commands Michael S. Tsirkin
2023-07-06 22:36   ` [virtio-comment] " Michael S. Tsirkin
2023-07-07 11:37   ` [virtio-dev] " Cornelia Huck
2023-07-07 11:37     ` [virtio-comment] " Cornelia Huck

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.