[virtio-comment] proposal: use admin command (and aq) of the device to query config space

[virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

One line proposal: 
Let's use new admin command and admin q for all device types to query device config space for new fields. (always).

Details below.

Query of device capabilities and configuration using DMA interface.
Need: 
Currently device configuration space is exposed as read only registers. It is growing rapidly.
Some devices may be even multi-functionality device in coming future such as net + clock + rdma device.
For a PCI transport implementing such ever-growing capabilities, configuration is burdensome as plain registers.
Hence, it is required for the driver to query capabilities and configuration using a DMA interface.

Interface requirements:
1. Maintain backward compatibility for existing defined configuration fields to stay as registers.
2. Any new field added must be accessed via DMA interface, regardless of device implementation (hw/sw etc).
Results in single driver code regardless of device implementation.
3. A device must be able to choose, starting from which field driver must query 
such configuration via DMA interface. This field offset must be greater than currently defined configuration field.
4. Any driver to device query operation must not be mandated to be
mediated by the owner device for PCI VFs or SIOV or SF devices. Driver must be able to
communicate query capabilities and configuration fields directly to
the device regardless of device type being PCI PF, VF, SF/SIOV device uniformly.
5. When having multi-functionality device in future, it is desired to not always
query all the configuration but may be able to query per-functionality configurations.
For example, query only steering capabilities, query only rdma capabilities or query only clock capabilities.
6. The driver should be able to query config/capabilities without
polling for the DMA completion, in other words, the driver should be able to get
notification from the device when DMA command completes.
7. The driver should be able to utilize existing interrupt vector and/or
virtqueue for query and set operation without demanding
additional interrupt vector whenever possible.

There are multiple options for DMA interface.
Some of these options are listed below that we would like to consider fulfilling above requirements.

Option_1:
New DMA interface registers.
New registers which allows single outstanding DMA command per device.
Such as,
struct pci_dma_cmd_mmio_registers {
	le64 cmd_addr; /* rw */
	le32 cmd_len;	/* rw: ordered write to it after cmd_addr, this triggers cmd to device */
	le32 start_offset_cfg_space; /* fields below this offset are not available as register, dma is must */
};

struct pci_config_access_cmd {
	u8 opcode; /* 0 = get config, 1 to N set dev specific config */
	u8 reserved;
	le16 msix_vector_index;
	le64 rsp_addr; /* points to struct pci_config_access_rsp */
	le32 rsp_len;
};
struct pci_config_access_rsp {
	u8 status;
	u8 debug_field;
	u8 cmd_specific_data[];
};

Cons:
1. Duplication of a VQ interface which can do same work and vq is purposed for "bulk data transfer" in spec already like this use case.
2. Some devices already have CVQ that can easily fulfil this role, which is not utilized.
3. Requires per device additional 16 bytes of register space.
4. Requires an extra msix interrupt vector to differentiate from config change interrupts.

Option_2:
Use admin vq for all the device types regardless of its transport such as PCI PF, PCI VF, PCI SIOV.
In this method a new admin command is issued on the admin vq of the device itself.
Pros:
1. Requirements 1 to 7 are fulfilled.
2. Driver can reuse the same vector with CVQ that addresses requirement #7.
Cons:
1. Requires per device unique admin queue number and count.
Still better than dedicated dma interface of #1, as it requires only 4 additional bytes as opposed to 12 bytes.

Option_3:
Use control VQ for the devices that already has CVQ.
In this method an existing CVQ of the device is used to query device config space.
Pros:
1. All 7 requirements are fulfilled.
2. Does not need 4 bytes of admin queue registers.
3. Save MSI-X vectors of the AQ.
4. Superior to option_2 as it doesn't require extra AQ.
5. Most efficient of all 3 options.
Cons:
1. Those devices which does not have CVQ, may need to add it if at all they are growing.

d. Any other option?

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/

[virtio-comment] Re: proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Tue, Aug 01, 2023 at 07:09:14AM +0000, Parav Pandit wrote:
> One line proposal: 
> Let's use new admin command and admin q for all device types to query device config space for new fields. (always).
> 
> Details below.
> 
> Query of device capabilities and configuration using DMA interface.
> Need: 
> Currently device configuration space is exposed as read only registers. It is growing rapidly.
> Some devices may be even multi-functionality device in coming future such as net + clock + rdma device.
> For a PCI transport implementing such ever-growing capabilities, configuration is burdensome as plain registers.
> Hence, it is required for the driver to query capabilities and configuration using a DMA interface.
> 
> Interface requirements:
> 1. Maintain backward compatibility for existing defined configuration fields to stay as registers.

This is both too specific and too vague.  I think you mean "device
should be able to optionally support both config access over DMA and support
existing transitional and non-transitional virtio pci drivers".

> 2. Any new field added must be accessed via DMA interface, regardless of device implementation (hw/sw etc).
> Results in single driver code regardless of device implementation.
> 3. A device must be able to choose, starting from which field driver must query 
> such configuration via DMA interface. This field offset must be greater than currently defined configuration field.

2 and 3 look like implementation more than like a requirement.
Try to think what your actual requirement is.

> 4. Any driver to device query operation must not be mandated to be
> mediated by the owner device for PCI VFs or SIOV or SF devices. Driver must be able to
> communicate query capabilities and configuration fields directly to
> the device regardless of device type being PCI PF, VF, SF/SIOV device uniformly.

I don't really know what does it mean to communicate directly with SIOV device.
neither do I know query is and what capabilities are.

> 5. When having multi-functionality device in future, it is desired to not always
> query all the configuration but may be able to query per-functionality configurations.
> For example, query only steering capabilities, query only rdma capabilities or query only clock capabilities.

I don't know what query is and what capabilities are. Do you mean
"access part of config space"?


> 6. The driver should be able to query config/capabilities without
> polling for the DMA completion, in other words, the driver should be able to get
> notification from the device when DMA command completes.
> 7. The driver should be able to utilize existing interrupt vector and/or
> virtqueue for query and set operation without demanding
> additional interrupt vector whenever possible.

additional over what? existing where?

> 
> There are multiple options for DMA interface.
> Some of these options are listed below that we would like to consider fulfilling above requirements.


Cc: Zhu Lingshan <lingshan.zhu@intel.com>

Zhu Lingshan are you still interested in a transport for SIOV (what was
called transport vq)? Clearly SIOV needs this as part of the transport.

-- 
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/

[virtio-comment] RE: proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, August 2, 2023 9:50 AM
> 
> On Tue, Aug 01, 2023 at 07:09:14AM +0000, Parav Pandit wrote:
> > One line proposal:
> > Let's use new admin command and admin q for all device types to query
> device config space for new fields. (always).
> >
> > Details below.
> >
> > Query of device capabilities and configuration using DMA interface.
> > Need:
> > Currently device configuration space is exposed as read only registers. It is
> growing rapidly.
> > Some devices may be even multi-functionality device in coming future such as
> net + clock + rdma device.
> > For a PCI transport implementing such ever-growing capabilities,
> configuration is burdensome as plain registers.
> > Hence, it is required for the driver to query capabilities and configuration
> using a DMA interface.
> >
> > Interface requirements:
> > 1. Maintain backward compatibility for existing defined configuration fields to
> stay as registers.
> 
> This is both too specific and too vague.  I think you mean "device should be able
> to optionally support both config access over DMA and support existing
> transitional and non-transitional virtio pci drivers".
>
Not really.
New fields always over DMA, 
for example bitmap for virtio-net counters must be via dma interface and upto hash report via register access.

> > 2. Any new field added must be accessed via DMA interface, regardless of
> device implementation (hw/sw etc).
> > Results in single driver code regardless of device implementation.
> > 3. A device must be able to choose, starting from which field driver
> > must query such configuration via DMA interface. This field offset must be
> greater than currently defined configuration field.
> 
> 2 and 3 look like implementation more than like a requirement.
> Try to think what your actual requirement is.
>
It is the actual requirement to not have them as register regardless of implementation.

 
> > 4. Any driver to device query operation must not be mandated to be
> > mediated by the owner device for PCI VFs or SIOV or SF devices. Driver
> > must be able to communicate query capabilities and configuration
> > fields directly to the device regardless of device type being PCI PF, VF, SF/SIOV
> device uniformly.
> 
> I don't really know what does it mean to communicate directly with SIOV
> device.
SIOV is not any special beast. It has its own VQ and registers like VFs etc.
Communicate directly means, any mediated PF driver is not involved in communication.
PF delegate some resource as passthrough to the SIOV device and PF role ends there.
PF driver does not get involved in per command, per access transport.

> neither do I know query is and what capabilities are.
>
Capabilities = roughly config space fields.
The reason I split it because dumping everything in a single config space structure is not good as fields gets sparced.
For example, Xuan adding some 64 bits for counter and tomorrow we may need more bits.
And Heng and I will add steering bits after that.
When half year later more counter bits needed, it will be located at some different place.

If one reserved, it may be still not enough, if reserved too much its unwanted wastage.
So capabilities is basically logically clubbed structure per functionality that one can query independently of other.

Like,
struct steering_capability {

};

struct counters_bitmap {

};

 > > 5. When having multi-functionality device in future, it is desired to
> > not always query all the configuration but may be able to query per-
> functionality configurations.
> > For example, query only steering capabilities, query only rdma capabilities or
> query only clock capabilities.
> 
> I don't know what query is and what capabilities are. Do you mean "access part
> of config space"?
>
Yes, I explained above.
 
> 
> > 6. The driver should be able to query config/capabilities without
> > polling for the DMA completion, in other words, the driver should be
> > able to get notification from the device when DMA command completes.
> > 7. The driver should be able to utilize existing interrupt vector
> > and/or virtqueue for query and set operation without demanding
> > additional interrupt vector whenever possible.
> 
> additional over what? existing where?
> 
Additional over existing usage.

> >
> > There are multiple options for DMA interface.
> > Some of these options are listed below that we would like to consider fulfilling
> above requirements.
> 
> 
> Cc: Zhu Lingshan <lingshan.zhu@intel.com>
> 
> Zhu Lingshan are you still interested in a transport for SIOV (what was called
> transport vq)? Clearly SIOV needs this as part of the transport.

Its not related to SIOV.
This proposal works for PF, VF, SIOV without any difference.
And possibly for non PCI transport too when done using VQ.

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/

[virtio-comment] Re: proposal: use admin command (and aq) of the device to query config space

[Zhu, Lingshan]

On 8/2/2023 12:20 PM, Michael S. Tsirkin wrote:
> On Tue, Aug 01, 2023 at 07:09:14AM +0000, Parav Pandit wrote:
>> One line proposal:
>> Let's use new admin command and admin q for all device types to query device config space for new fields. (always).
>>
>> Details below.
>>
>> Query of device capabilities and configuration using DMA interface.
>> Need:
>> Currently device configuration space is exposed as read only registers. It is growing rapidly.
>> Some devices may be even multi-functionality device in coming future such as net + clock + rdma device.
>> For a PCI transport implementing such ever-growing capabilities, configuration is burdensome as plain registers.
>> Hence, it is required for the driver to query capabilities and configuration using a DMA interface.
>>
>> Interface requirements:
>> 1. Maintain backward compatibility for existing defined configuration fields to stay as registers.
> This is both too specific and too vague.  I think you mean "device
> should be able to optionally support both config access over DMA and support
> existing transitional and non-transitional virtio pci drivers".
>
>> 2. Any new field added must be accessed via DMA interface, regardless of device implementation (hw/sw etc).
>> Results in single driver code regardless of device implementation.
>> 3. A device must be able to choose, starting from which field driver must query
>> such configuration via DMA interface. This field offset must be greater than currently defined configuration field.
> 2 and 3 look like implementation more than like a requirement.
> Try to think what your actual requirement is.
>
>> 4. Any driver to device query operation must not be mandated to be
>> mediated by the owner device for PCI VFs or SIOV or SF devices. Driver must be able to
>> communicate query capabilities and configuration fields directly to
>> the device regardless of device type being PCI PF, VF, SF/SIOV device uniformly.
> I don't really know what does it mean to communicate directly with SIOV device.
> neither do I know query is and what capabilities are.
>
>> 5. When having multi-functionality device in future, it is desired to not always
>> query all the configuration but may be able to query per-functionality configurations.
>> For example, query only steering capabilities, query only rdma capabilities or query only clock capabilities.
> I don't know what query is and what capabilities are. Do you mean
> "access part of config space"?
>
>
>> 6. The driver should be able to query config/capabilities without
>> polling for the DMA completion, in other words, the driver should be able to get
>> notification from the device when DMA command completes.
>> 7. The driver should be able to utilize existing interrupt vector and/or
>> virtqueue for query and set operation without demanding
>> additional interrupt vector whenever possible.
> additional over what? existing where?
>
>> There are multiple options for DMA interface.
>> Some of these options are listed below that we would like to consider fulfilling above requirements.
>
> Cc: Zhu Lingshan <lingshan.zhu@intel.com>
>
> Zhu Lingshan are you still interested in a transport for SIOV (what was
> called transport vq)? Clearly SIOV needs this as part of the transport.
Hi Michael,

Rebasing "transport vq" on admin vq is still in my pipeline, I am 
currently working
on a patch series implementing virtio pci live migration in the 
spec(carry on Jason and Eugenio's work).
I will post it next week and back to transport vq task then.

For SIOV, I have implemented the interfaces for querying the 
configurations, for example:
1) Access config space:

247 +\begin{lstlisting}
248 +#define VIRTIO_TRANSPTQ_CTRL_CONFIG    6
249 +  #define VIRTIO_TRANSPTQ_CTRL_CONFIG_GET    0
250 +  #define VIRTIO_TRANSPTQ_CTRL_CONFIG_SET    1
251 +
252 +struct virtio_transportq_ctrl_dev_config_get {
253 +       u32 offset;
254 +       u32 size;
255 +};
256 +
257 +struct virtio_transportq_ctrl_dev_config_set {
258 +       u32 offset;
259 +       u32 size;
260 +       u8  data[];
261 +};
262 +\end{lstlisting}

I think we can re-use the GET command for SR-IOV, I am afraid SET would 
be a side-channel attacking surface for SR-IOV.

2) a specific config, e.g., features:

142 +The features negotiation of managed devices is done by the
143 +following commands:
144 +
145 +\begin{lstlisting}
146 +#define VIRTIO_TRANSPTQ_CTRL_FEAT   3
147 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DEVICE_GET        0
148 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DRIVER_SET        1
149 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DRIVER_GET        2
150 +
151 +struct virtio_transportq_ctrl_dev_features {
152 +        u32 features_len;
153 +        u32 features[features_len];
154 +};
155 +\end{lstlisting}

still can re-use the GET commands.

Thanks
Zhu Lingshan
>


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/

Re: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Jason Wang]

On Tue, Aug 1, 2023 at 3:09 PM Parav Pandit <parav@nvidia.com> wrote:
>
> One line proposal:
> Let's use new admin command and admin q for all device types to query device config space for new fields. (always).

Before we mandate anything to admin command, we need to first invent
an admin command over MMIO interface otherwise it would always be an
issue for the nesting.

>
> Details below.
>
> Query of device capabilities and configuration using DMA interface.
> Need:
> Currently device configuration space is exposed as read only registers.

This is wrong:

1) device configuration space is transport independent, some transport
already use DMA to access the device configuration space
2) device configuration space is not read only, we've already had
several examples of using it as write

> It is growing rapidly.
> Some devices may be even multi-functionality device in coming future such as net + clock + rdma device.
> For a PCI transport implementing such ever-growing capabilities, configuration is burdensome as plain registers.

We've already fixed size VIRTIO_PCI_CAP_PCI_CFG. What's wrong with that?

And we have a lot of device specific virtqueues that could be used for
configuration.

> Hence, it is required for the driver to query capabilities and configuration using a DMA interface.
>
> Interface requirements:
> 1. Maintain backward compatibility for existing defined configuration fields to stay as registers.
> 2. Any new field added must be accessed via DMA interface, regardless of device implementation (hw/sw etc).
> Results in single driver code regardless of device implementation.

Virtio is flexible as it decouples transport from the device model.
This breaks this flexibility, and this prevents non-DMA transport from
being developed.

> 3. A device must be able to choose, starting from which field driver must query
> such configuration via DMA interface. This field offset must be greater than currently defined configuration field.
> 4. Any driver to device query operation must not be mandated to be
> mediated by the owner device for PCI VFs or SIOV or SF devices. Driver must be able to
> communicate query capabilities and configuration fields directly to
> the device regardless of device type being PCI PF, VF, SF/SIOV device uniformly.
> 5. When having multi-functionality device in future, it is desired to not always
> query all the configuration but may be able to query per-functionality configurations.
> For example, query only steering capabilities, query only rdma capabilities or query only clock capabilities.
> 6. The driver should be able to query config/capabilities without
> polling for the DMA completion, in other words, the driver should be able to get
> notification from the device when DMA command completes.
> 7. The driver should be able to utilize existing interrupt vector and/or
> virtqueue for query and set operation without demanding
> additional interrupt vector whenever possible.
>
> There are multiple options for DMA interface.
> Some of these options are listed below that we would like to consider fulfilling above requirements.
>
> Option_1:
> New DMA interface registers.
> New registers which allows single outstanding DMA command per device.
> Such as,
> struct pci_dma_cmd_mmio_registers {
>         le64 cmd_addr; /* rw */
>         le32 cmd_len;   /* rw: ordered write to it after cmd_addr, this triggers cmd to device */
>         le32 start_offset_cfg_space; /* fields below this offset are not available as register, dma is must */
> };
>
> struct pci_config_access_cmd {
>         u8 opcode; /* 0 = get config, 1 to N set dev specific config */
>         u8 reserved;
>         le16 msix_vector_index;
>         le64 rsp_addr; /* points to struct pci_config_access_rsp */
>         le32 rsp_len;
> };
> struct pci_config_access_rsp {
>         u8 status;
>         u8 debug_field;
>         u8 cmd_specific_data[];
> };
>
> Cons:
> 1. Duplication of a VQ interface which can do same work and vq is purposed for "bulk data transfer" in spec already like this use case.
> 2. Some devices already have CVQ that can easily fulfil this role, which is not utilized.
> 3. Requires per device additional 16 bytes of register space.
> 4. Requires an extra msix interrupt vector to differentiate from config change interrupts.
>
> Option_2:
> Use admin vq for all the device types regardless of its transport such as PCI PF, PCI VF, PCI SIOV.
> In this method a new admin command is issued on the admin vq of the device itself.
> Pros:
> 1. Requirements 1 to 7 are fulfilled.
> 2. Driver can reuse the same vector with CVQ that addresses requirement #7.
> Cons:
> 1. Requires per device unique admin queue number and count.
> Still better than dedicated dma interface of #1, as it requires only 4 additional bytes as opposed to 12 bytes.
>
> Option_3:
> Use control VQ for the devices that already has CVQ.
> In this method an existing CVQ of the device is used to query device config space.
> Pros:
> 1. All 7 requirements are fulfilled.
> 2. Does not need 4 bytes of admin queue registers.
> 3. Save MSI-X vectors of the AQ.
> 4. Superior to option_2 as it doesn't require extra AQ.
> 5. Most efficient of all 3 options.
> Cons:
> 1. Those devices which does not have CVQ, may need to add it if at all they are growing.
>
> d. Any other option?

Transport virtqueue on top of admin virtqueue will address this seamlessly.

Thanks


>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription is required
> before posting.
>
> Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> List help: virtio-comment-help@lists.oasis-open.org
> List archive: https://lists.oasis-open.org/archives/virtio-comment/
> Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> Committee: https://www.oasis-open.org/committees/virtio/
> Join OASIS: https://www.oasis-open.org/join/
>


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/

[virtio-comment] RE: proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Wednesday, August 2, 2023 2:15 PM
> Rebasing "transport vq" on admin vq is still in my pipeline, I am currently
> working on a patch series implementing virtio pci live migration in the
> spec(carry on Jason and Eugenio's work).
> I will post it next week and back to transport vq task then.
> 
Awesome and interesting to us as well.
I have draft for member device migration admin commands.
I guess yours will hit the mailing list first, so we can converge on your series.

> For SIOV, I have implemented the interfaces for querying the configurations, for
> example:
> 1) Access config space:
> 
> 247 +\begin{lstlisting}
> 248 +#define VIRTIO_TRANSPTQ_CTRL_CONFIG    6
> 249 +  #define VIRTIO_TRANSPTQ_CTRL_CONFIG_GET    0
> 250 +  #define VIRTIO_TRANSPTQ_CTRL_CONFIG_SET    1
> 251 +
> 252 +struct virtio_transportq_ctrl_dev_config_get {
> 253 +       u32 offset;
> 254 +       u32 size;
> 255 +};
> 256 +
> 257 +struct virtio_transportq_ctrl_dev_config_set {
> 258 +       u32 offset;
> 259 +       u32 size;
> 260 +       u8  data[];
> 261 +};
> 262 +\end{lstlisting}
> 
> I think we can re-use the GET command for SR-IOV, I am afraid SET would be a
> side-channel attacking surface for SR-IOV.
> 
Get and set both should be inband for PF, VF, SIOV.
I don’t see any need to diverge and create any weird combinations differently for 3 different device types.

> 2) a specific config, e.g., features:
> 
> 142 +The features negotiation of managed devices is done by the
> 143 +following commands:
> 144 +
> 145 +\begin{lstlisting}
> 146 +#define VIRTIO_TRANSPTQ_CTRL_FEAT   3
> 147 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DEVICE_GET        0
> 148 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DRIVER_SET        1
> 149 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DRIVER_GET        2
> 150 +
> 151 +struct virtio_transportq_ctrl_dev_features {
> 152 +        u32 features_len;
> 153 +        u32 features[features_len];
> 154 +};
> 155 +\end{lstlisting}
> 
> still can re-use the GET commands.
> 
> Thanks
> Zhu Lingshan
> >

RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, August 2, 2023 2:23 PM
> 
> On Tue, Aug 1, 2023 at 3:09 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> > One line proposal:
> > Let's use new admin command and admin q for all device types to query
> device config space for new fields. (always).
> 
> Before we mandate anything to admin command, we need to first invent an
> admin command over MMIO interface otherwise it would always be an issue
> for the nesting.
> 
Nesting can be independent requirement in itself.
> >
> > Details below.
> >
> > Query of device capabilities and configuration using DMA interface.
> > Need:
> > Currently device configuration space is exposed as read only registers.
> 
> This is wrong:
> 
> 1) device configuration space is transport independent, some transport already
> use DMA to access the device configuration space
You can say ccw instead of "some". :)

> 2) device configuration space is not read only, we've already had several
> examples of using it as write
> 
It is even worse to have writable.

> > It is growing rapidly.
> > Some devices may be even multi-functionality device in coming future such as
> net + clock + rdma device.
> > For a PCI transport implementing such ever-growing capabilities,
> configuration is burdensome as plain registers.
> 
> We've already fixed size VIRTIO_PCI_CAP_PCI_CFG. What's wrong with that?
> 
The wrong part is: it is still and indirect and slow, sub-optimal register interface.

> And we have a lot of device specific virtqueues that could be used for
> configuration.
> 
Sure, this is the option_3 listed in here.

> > Hence, it is required for the driver to query capabilities and configuration
> using a DMA interface.
> >
> > Interface requirements:
> > 1. Maintain backward compatibility for existing defined configuration fields to
> stay as registers.
> > 2. Any new field added must be accessed via DMA interface, regardless of
> device implementation (hw/sw etc).
> > Results in single driver code regardless of device implementation.
> 
> Virtio is flexible as it decouples transport from the device model.
> This breaks this flexibility, and this prevents non-DMA transport from being
> developed.
> 
VQ is decoupled from transport already.
So, there is no flexibility broken.
And yet you suggested transport dependent VIRTIO_PCI_CAP_PCI_CFG above making it further wrong. :)

> > 3. A device must be able to choose, starting from which field driver
> > must query such configuration via DMA interface. This field offset must be
> greater than currently defined configuration field.
> > 4. Any driver to device query operation must not be mandated to be
> > mediated by the owner device for PCI VFs or SIOV or SF devices. Driver
> > must be able to communicate query capabilities and configuration
> > fields directly to the device regardless of device type being PCI PF, VF, SF/SIOV
> device uniformly.
> > 5. When having multi-functionality device in future, it is desired to
> > not always query all the configuration but may be able to query per-
> functionality configurations.
> > For example, query only steering capabilities, query only rdma capabilities or
> query only clock capabilities.
> > 6. The driver should be able to query config/capabilities without
> > polling for the DMA completion, in other words, the driver should be
> > able to get notification from the device when DMA command completes.
> > 7. The driver should be able to utilize existing interrupt vector
> > and/or virtqueue for query and set operation without demanding
> > additional interrupt vector whenever possible.
> >
> > There are multiple options for DMA interface.
> > Some of these options are listed below that we would like to consider fulfilling
> above requirements.
> >
> > Option_1:
> > New DMA interface registers.
> > New registers which allows single outstanding DMA command per device.
> > Such as,
> > struct pci_dma_cmd_mmio_registers {
> >         le64 cmd_addr; /* rw */
> >         le32 cmd_len;   /* rw: ordered write to it after cmd_addr, this triggers
> cmd to device */
> >         le32 start_offset_cfg_space; /* fields below this offset are
> > not available as register, dma is must */ };
> >
> > struct pci_config_access_cmd {
> >         u8 opcode; /* 0 = get config, 1 to N set dev specific config */
> >         u8 reserved;
> >         le16 msix_vector_index;
> >         le64 rsp_addr; /* points to struct pci_config_access_rsp */
> >         le32 rsp_len;
> > };
> > struct pci_config_access_rsp {
> >         u8 status;
> >         u8 debug_field;
> >         u8 cmd_specific_data[];
> > };
> >
> > Cons:
> > 1. Duplication of a VQ interface which can do same work and vq is purposed
> for "bulk data transfer" in spec already like this use case.
> > 2. Some devices already have CVQ that can easily fulfil this role, which is not
> utilized.
> > 3. Requires per device additional 16 bytes of register space.
> > 4. Requires an extra msix interrupt vector to differentiate from config change
> interrupts.
> >
> > Option_2:
> > Use admin vq for all the device types regardless of its transport such as PCI PF,
> PCI VF, PCI SIOV.
> > In this method a new admin command is issued on the admin vq of the device
> itself.
> > Pros:
> > 1. Requirements 1 to 7 are fulfilled.
> > 2. Driver can reuse the same vector with CVQ that addresses requirement #7.
> > Cons:
> > 1. Requires per device unique admin queue number and count.
> > Still better than dedicated dma interface of #1, as it requires only 4 additional
> bytes as opposed to 12 bytes.
> >
> > Option_3:
> > Use control VQ for the devices that already has CVQ.
> > In this method an existing CVQ of the device is used to query device config
> space.
> > Pros:
> > 1. All 7 requirements are fulfilled.
> > 2. Does not need 4 bytes of admin queue registers.
> > 3. Save MSI-X vectors of the AQ.
> > 4. Superior to option_2 as it doesn't require extra AQ.
> > 5. Most efficient of all 3 options.
> > Cons:
> > 1. Those devices which does not have CVQ, may need to add it if at all they
> are growing.
> >
> > d. Any other option?
> 
> Transport virtqueue on top of admin virtqueue will address this seamlessly.
> 
:)

Don’t see why one would create few more objects on top of aq when aq or cvq itself can fulfil the need.
Can you please elaborate?

[virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Jason Wang]

On Wed, Aug 2, 2023 at 5:07 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, August 2, 2023 2:23 PM
> >
> > On Tue, Aug 1, 2023 at 3:09 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > One line proposal:
> > > Let's use new admin command and admin q for all device types to query
> > device config space for new fields. (always).
> >
> > Before we mandate anything to admin command, we need to first invent an
> > admin command over MMIO interface otherwise it would always be an issue
> > for the nesting.
> >
> Nesting can be independent requirement in itself.'

I don't understand here. If you tie new fields to the DMA interface,
it basically means nesting won't get any new features unless:

1) it's a PCI VF
2) SR-IOV emulation is done
3) admin virtqueue emulation is done

If you want differ nesting devices from others, it would be a
nightmare to maintain.

> > >
> > > Details below.
> > >
> > > Query of device capabilities and configuration using DMA interface.
> > > Need:
> > > Currently device configuration space is exposed as read only registers.
> >
> > This is wrong:
> >
> > 1) device configuration space is transport independent, some transport already
> > use DMA to access the device configuration space
> You can say ccw instead of "some". :)

Kind of but the transport vq proposal goes in the same way.

>
> > 2) device configuration space is not read only, we've already had several
> > examples of using it as write
> >
> It is even worse to have writable.

Well, what I meant is that, it's not necessarily read only and not
necessarily a register interface.

>
> > > It is growing rapidly.
> > > Some devices may be even multi-functionality device in coming future such as
> > net + clock + rdma device.
> > > For a PCI transport implementing such ever-growing capabilities,
> > configuration is burdensome as plain registers.
> >
> > We've already fixed size VIRTIO_PCI_CAP_PCI_CFG. What's wrong with that?
> >
> The wrong part is: it is still and indirect and slow, sub-optimal register interface.

Do we really care about the performance here? And if it is one of your
major concerns, it's better to explain it along with the
"ever-growing" concern.

>
> > And we have a lot of device specific virtqueues that could be used for
> > configuration.
> >
> Sure, this is the option_3 listed in here.
>
> > > Hence, it is required for the driver to query capabilities and configuration
> > using a DMA interface.
> > >
> > > Interface requirements:
> > > 1. Maintain backward compatibility for existing defined configuration fields to
> > stay as registers.
> > > 2. Any new field added must be accessed via DMA interface, regardless of
> > device implementation (hw/sw etc).
> > > Results in single driver code regardless of device implementation.
> >
> > Virtio is flexible as it decouples transport from the device model.
> > This breaks this flexibility, and this prevents non-DMA transport from being
> > developed.
> >
> VQ is decoupled from transport already.
> So, there is no flexibility broken.
> And yet you suggested transport dependent VIRTIO_PCI_CAP_PCI_CFG above making it further wrong. :)
>

The context here is that you want to mandate any new fields to be DMA.
DMA is obviously transport specific. There are transports that don't
use DMA at all (e.g the shared memory).

> > > 3. A device must be able to choose, starting from which field driver
> > > must query such configuration via DMA interface. This field offset must be
> > greater than currently defined configuration field.

[...]

> > >
> > > d. Any other option?
> >
> > Transport virtqueue on top of admin virtqueue will address this seamlessly.
> >
> :)
>
> Don’t see why one would create few more objects on top of aq when aq or cvq itself can fulfil the need.
> Can you please elaborate?

If cvq can work, there's no need for any other methods. If you care
about admin virtqueue then device configuration space is not the only
thing that can be "ever growing", common_cfg is another one. Allowing
them to be accessed via a virtqueue (thus DMA in PCI transport) is
basically the idea of the transport virtqueue. Ling shan is rebasing
transport virtqueue proposals on top of admin commands, we can see
then.

Thanks


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

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

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

[virtio-comment] Re: proposal: use admin command (and aq) of the device to query config space

[Zhu, Lingshan]

On 8/2/2023 4:59 PM, Parav Pandit wrote:
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Wednesday, August 2, 2023 2:15 PM
>> Rebasing "transport vq" on admin vq is still in my pipeline, I am currently
>> working on a patch series implementing virtio pci live migration in the
>> spec(carry on Jason and Eugenio's work).
>> I will post it next week and back to transport vq task then.
>>
> Awesome and interesting to us as well.
> I have draft for member device migration admin commands.
> I guess yours will hit the mailing list first, so we can converge on your series.
OK
>
>> For SIOV, I have implemented the interfaces for querying the configurations, for
>> example:
>> 1) Access config space:
>>
>> 247 +\begin{lstlisting}
>> 248 +#define VIRTIO_TRANSPTQ_CTRL_CONFIG    6
>> 249 +  #define VIRTIO_TRANSPTQ_CTRL_CONFIG_GET    0
>> 250 +  #define VIRTIO_TRANSPTQ_CTRL_CONFIG_SET    1
>> 251 +
>> 252 +struct virtio_transportq_ctrl_dev_config_get {
>> 253 +       u32 offset;
>> 254 +       u32 size;
>> 255 +};
>> 256 +
>> 257 +struct virtio_transportq_ctrl_dev_config_set {
>> 258 +       u32 offset;
>> 259 +       u32 size;
>> 260 +       u8  data[];
>> 261 +};
>> 262 +\end{lstlisting}
>>
>> I think we can re-use the GET command for SR-IOV, I am afraid SET would be a
>> side-channel attacking surface for SR-IOV.
>>
> Get and set both should be inband for PF, VF, SIOV.
> I don’t see any need to diverge and create any weird combinations differently for 3 different device types.
I am not sure, when passthrough a device to a guest, the guest driver 
ownes the config space of the device,
if we allow changes against the config space through admin vq from host, 
it looks like a risk.
>
>> 2) a specific config, e.g., features:
>>
>> 142 +The features negotiation of managed devices is done by the
>> 143 +following commands:
>> 144 +
>> 145 +\begin{lstlisting}
>> 146 +#define VIRTIO_TRANSPTQ_CTRL_FEAT   3
>> 147 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DEVICE_GET        0
>> 148 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DRIVER_SET        1
>> 149 + #define VIRTIO_TRANSPTQ_CTRL_FEAT_DRIVER_GET        2
>> 150 +
>> 151 +struct virtio_transportq_ctrl_dev_features {
>> 152 +        u32 features_len;
>> 153 +        u32 features[features_len];
>> 154 +};
>> 155 +\end{lstlisting}
>>
>> still can re-use the GET commands.
>>
>> Thanks
>> Zhu Lingshan


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/

[virtio-comment] RE: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, August 2, 2023 3:02 PM
> 
> On Wed, Aug 2, 2023 at 5:07 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, August 2, 2023 2:23 PM
> > >
> > > On Tue, Aug 1, 2023 at 3:09 PM Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > > One line proposal:
> > > > Let's use new admin command and admin q for all device types to
> > > > query
> > > device config space for new fields. (always).
> > >
> > > Before we mandate anything to admin command, we need to first invent
> > > an admin command over MMIO interface otherwise it would always be an
> > > issue for the nesting.
> > >
> > Nesting can be independent requirement in itself.'
> 
> I don't understand here. If you tie new fields to the DMA interface, it basically
> means nesting won't get any new features unless:
> 
> 1) it's a PCI VF
> 2) SR-IOV emulation is done
> 3) admin virtqueue emulation is done
> 
> If you want differ nesting devices from others, it would be a nightmare to
> maintain.
>
New fields for sure are tied to the DMA interface.
And nesting can also get just like how a VQ works in nested mode.
 
> > >
> > > 1) device configuration space is transport independent, some
> > > transport already use DMA to access the device configuration space
> > You can say ccw instead of "some". :)
> 
> Kind of but the transport vq proposal goes in the same way.
>
We debated many times that the wording transport vq is wrong as it is _not_ going to transport driver notifications.
Anyway, there is nothing to discuss here. So focusing on main items below.
 
> >
> > > 2) device configuration space is not read only, we've already had
> > > several examples of using it as write
> > >
> > It is even worse to have writable.
> 
> Well, what I meant is that, it's not necessarily read only and not necessarily a
> register interface.
>
I took PCI as being most common interface and took net, blk as devices who are experiencing high growth on features and device specific config space.

This isn’t really a normative part of the spec. The key takeway to have it, for common things it is read only and its register.
 
> >
> > > > It is growing rapidly.
> > > > Some devices may be even multi-functionality device in coming
> > > > future such as
> > > net + clock + rdma device.
> > > > For a PCI transport implementing such ever-growing capabilities,
> > > configuration is burdensome as plain registers.
> > >
> > > We've already fixed size VIRTIO_PCI_CAP_PCI_CFG. What's wrong with that?
> > >
> > The wrong part is: it is still and indirect and slow, sub-optimal register
> interface.
> 
> Do we really care about the performance here? 
When it comes to bulk data transfer, in range of few hundred bytes and looking for 5+ year period, than yes, reading using indirect register is slow.
And after all it is still indirect register which is does not have time variability part.
So it is not fulfilling the requirement at all.

> And if it is one of your major
> concerns, it's better to explain it along with the "ever-growing" concern.
>
I thought it is clear that it is still a register, even slower than current one and still have same issues without it.

> > VQ is decoupled from transport already.
> > So, there is no flexibility broken.
> > And yet you suggested transport dependent VIRTIO_PCI_CAP_PCI_CFG above
> > making it further wrong. :)
> >
> 
> The context here is that you want to mandate any new fields to be DMA.
> DMA is obviously transport specific. There are transports that don't use DMA at
> all (e.g the shared memory).
>

VQ is surprisingly does the DMA without being transport specific.
How net device mandate tx pkts via vq, how a console device mandate receive and transmit queue,
How a crypto device mandate a control vq,

What is proposed here is no different...

Shared memory is not for bulk data transfer in virtio spec.
We don’t see "shared memory" as transport in " Virtio Transport Options" section.
 
> > > > 3. A device must be able to choose, starting from which field
> > > > driver must query such configuration via DMA interface. This field
> > > > offset must be
> > > greater than currently defined configuration field.
> 
> [...]
> 
> > > >
> > > > d. Any other option?
> > >
> > > Transport virtqueue on top of admin virtqueue will address this seamlessly.
> > >
> > :)
> >
> > Don’t see why one would create few more objects on top of aq when aq or
> cvq itself can fulfil the need.
> > Can you please elaborate?
> 
> If cvq can work, there's no need for any other methods.
Cvq is not present for all the device, at the same time all devices are not experiencing high growth of config space either.
Hence the discussion here.
Michael raised the concern to use some generic scheme for all the device previously, hence the aq.

I think the practical approach is, whichever device has cvq, they should be able to use cvq for config+caps query+set.
And rest can burn AQ to transport the same command.

> If you care about admin
> virtqueue then device configuration space is not the only thing that can be
> "ever growing", common_cfg is another one. 
The idea is to do minimal bootstrap work from the common config space and switch to the queue.
So common config shouldn’t be growing either other than minimal bootstrap functionality.
Hence, common config also to be available via dma command.

> Allowing them to be accessed via a
> virtqueue (thus DMA in PCI transport) is basically the idea of the transport
> virtqueue. Ling shan is rebasing transport virtqueue proposals on top of admin
> commands, we can see then.

Current proposal doesnt need yet another new transport vq with/without nesting.
We just need to make choice as _always_admin command or efficiently use cvq when available.

[virtio-comment] RE: proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Wednesday, August 2, 2023 3:11 PM

> > Get and set both should be inband for PF, VF, SIOV.
> > I don’t see any need to diverge and create any weird combinations differently
> for 3 different device types.
> I am not sure, when passthrough a device to a guest, the guest driver ownes the
> config space of the device, if we allow changes against the config space through
> admin vq from host, it looks like a risk.

I said the opposite above, get and set is INBAND from guest driver to device for PF, VF and SIOV through its own delegated resource.
There is no AQ mediation from host.
Hence, there is no risk.

[virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 09:57:47AM +0000, Parav Pandit wrote:
> > If you care about admin
> > virtqueue then device configuration space is not the only thing that can be
> > "ever growing", common_cfg is another one. 
> The idea is to do minimal bootstrap work from the common config space and switch to the queue.
> So common config shouldn’t be growing either other than minimal bootstrap functionality.
> Hence, common config also to be available via dma command.

That is more like a new transport. I thought this proposal is focusing
on device specific config - let's get a handle on that first?
"common config" is a pci transport specific thing.


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/

[virtio-comment] RE: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, August 2, 2023 4:56 PM
> 
> On Wed, Aug 02, 2023 at 09:57:47AM +0000, Parav Pandit wrote:
> > > If you care about admin
> > > virtqueue then device configuration space is not the only thing that
> > > can be "ever growing", common_cfg is another one.
> > The idea is to do minimal bootstrap work from the common config space and
> switch to the queue.
> > So common config shouldn’t be growing either other than minimal bootstrap
> functionality.
> > Hence, common config also to be available via dma command.
> 
> That is more like a new transport. I thought this proposal is focusing on device
> specific config - let's get a handle on that first?
> "common config" is a pci transport specific thing.

Sure, device config is the real pain point we are trying to solve first.

Using cvq for those devices who has it seems the most optimal approach.
If we liberate ourselves from single monolithic config space structure and move to query device capabilities, resources, configuration, at functionality level, life is lot easier.
What are your thoughts?

[virtio-comment] Re: proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 10:00:09AM +0000, Parav Pandit wrote:
> 
> 
> > From: Zhu, Lingshan <lingshan.zhu@intel.com>
> > Sent: Wednesday, August 2, 2023 3:11 PM
> 
> > > Get and set both should be inband for PF, VF, SIOV.
> > > I don’t see any need to diverge and create any weird combinations differently
> > for 3 different device types.
> > I am not sure, when passthrough a device to a guest, the guest driver ownes the
> > config space of the device, if we allow changes against the config space through
> > admin vq from host, it looks like a risk.
> 
> I said the opposite above, get and set is INBAND from guest driver to device for PF, VF and SIOV through its own delegated resource.
> There is no AQ mediation from host.
> Hence, there is no risk.

I think there are different requirements from different people.  For
example, consider compatibility. I know it looks like super important
right now, and we should make it possible, but it's just like with
legacy interface - looked very far away when we introduced
non-transitional, to the point where an option in linux to disable
legacy talks about "flying cars", but the flying cars are nowhere to be
seen and, while we are not where everyone can just drop pre-1.0 bits, a
bunch of people are using e.g. vdpa without any legacy, express devices
in qemu also disabled legacy, and so on.


-- 
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/

[virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 11:29:53AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, August 2, 2023 4:56 PM
> > 
> > On Wed, Aug 02, 2023 at 09:57:47AM +0000, Parav Pandit wrote:
> > > > If you care about admin
> > > > virtqueue then device configuration space is not the only thing that
> > > > can be "ever growing", common_cfg is another one.
> > > The idea is to do minimal bootstrap work from the common config space and
> > switch to the queue.
> > > So common config shouldn’t be growing either other than minimal bootstrap
> > functionality.
> > > Hence, common config also to be available via dma command.
> > 
> > That is more like a new transport. I thought this proposal is focusing on device
> > specific config - let's get a handle on that first?
> > "common config" is a pci transport specific thing.
> 
> Sure, device config is the real pain point we are trying to solve first.
> 
> Using cvq for those devices who has it seems the most optimal approach.
> If we liberate ourselves from single monolithic config space structure and move to query device capabilities, resources, configuration, at functionality level, life is lot easier.
> What are your thoughts?

Splitting transport and device config is exactly what I'm talking about.
I agree transport should probably be split further -
it only made sense for legacy so we don't need to spend
specification effort on legacy.
splitting device config would require changes to all devices -
I don't see how it's worth the effort.

-- 
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/

RE: [virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> open.org> On Behalf Of Michael S. Tsirkin
> Sent: Wednesday, August 2, 2023 5:09 PM


> > Sure, device config is the real pain point we are trying to solve first.
> >
> > Using cvq for those devices who has it seems the most optimal approach.
> > If we liberate ourselves from single monolithic config space structure and
> move to query device capabilities, resources, configuration, at functionality
> level, life is lot easier.
> > What are your thoughts?
> 
> Splitting transport and device config is exactly what I'm talking about.
> I agree transport should probably be split further - it only made sense for legacy
> so we don't need to spend specification effort on legacy.
> splitting device config would require changes to all devices - I don't see how it's
> worth the effort.

Maybe I was not clear in my idea.
We have canned ourselves as config means _one_ structure.
Due to this thought process, all these transport and things muddy the view.

If one think of functionality-based config, there is no one structure, hence no need to limit ourselves to it.
Taking concrete example,

We have separate commands for,
a. RSS config
b. filters
c. notification coalescing

When you have matching get command, then each functionality grows by their own get command and no need to put in single box of single config structure.

Every device will be able to grow to dynamic need as/if it arise.

Re: [virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 11:44:46AM +0000, Parav Pandit wrote:
> 
> 
> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
> > open.org> On Behalf Of Michael S. Tsirkin
> > Sent: Wednesday, August 2, 2023 5:09 PM
> 
> 
> > > Sure, device config is the real pain point we are trying to solve first.
> > >
> > > Using cvq for those devices who has it seems the most optimal approach.
> > > If we liberate ourselves from single monolithic config space structure and
> > move to query device capabilities, resources, configuration, at functionality
> > level, life is lot easier.
> > > What are your thoughts?
> > 
> > Splitting transport and device config is exactly what I'm talking about.
> > I agree transport should probably be split further - it only made sense for legacy
> > so we don't need to spend specification effort on legacy.
> > splitting device config would require changes to all devices - I don't see how it's
> > worth the effort.
> 
> Maybe I was not clear in my idea.
> We have canned ourselves as config means _one_ structure.
> Due to this thought process, all these transport and things muddy the view.
> 
> If one think of functionality-based config, there is no one structure, hence no need to limit ourselves to it.
> Taking concrete example,
> 
> We have separate commands for,
> a. RSS config
> b. filters
> c. notification coalescing
> 
> When you have matching get command, then each functionality grows by their own get command and no need to put in single box of single config structure.
> 
> Every device will be able to grow to dynamic need as/if it arise.
> 
> 
> 
> 

There's one thing to say about putting everything in one place,
and that is that one can then find everything in one place.
For example, at the moment we actually do have a problem with cvq,
and the problem is that it sets internal device state
that is not observable (unlike original pre-1.0 config
space which was writeable).


By the way there's a pci express ECN for relaxed ordering or something
like this. I am yet to look at it, I wonder whether it can be
used to avoid the issues we have with MMIO in a way
that is easier to use than DMA.


-- 
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/

[virtio-comment] RE: proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, August 2, 2023 5:05 PM

> > I said the opposite above, get and set is INBAND from guest driver to device
> for PF, VF and SIOV through its own delegated resource.
> > There is no AQ mediation from host.
> > Hence, there is no risk.
> 
> I think there are different requirements from different people.  
Yes.
There are two requirements.
1. Build SIOV device that can live in hypervisor for having 1.x based PCI device in guest

2. Build SIOV device ground up that does not need to have existing limitations of 1.x.

The transport VQ proposal 4 patches had heavy inclination for #1 without mentioning the real objective in the cover letter.
I was proposing that when virtio builds SIOV device it should start with #2.
Because #1 is sort of handled with sr-iov or vdpa already.

#2 is more critical to get it right, after SIOV R2 spec is defined.


> For example,
> consider compatibility. I know it looks like super important right now, and we
> should make it possible, but it's just like with legacy interface - looked very far
> away when we introduced non-transitional, to the point where an option in
> linux to disable legacy talks about "flying cars", but the flying cars are nowhere
> to be seen and, while we are not where everyone can just drop pre-1.0 bits, a
> bunch of people are using e.g. vdpa without any legacy, express devices in
> qemu also disabled legacy, and so on.
> 
> 
> --
> MST

[virtio-comment] Re: proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 11:51:21AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, August 2, 2023 5:05 PM
> 
> > > I said the opposite above, get and set is INBAND from guest driver to device
> > for PF, VF and SIOV through its own delegated resource.
> > > There is no AQ mediation from host.
> > > Hence, there is no risk.
> > 
> > I think there are different requirements from different people.  
> Yes.
> There are two requirements.
> 1. Build SIOV device that can live in hypervisor for having 1.x based PCI device in guest
> 
> 2. Build SIOV device ground up that does not need to have existing limitations of 1.x.
> 
> The transport VQ proposal 4 patches had heavy inclination for #1 without mentioning the real objective in the cover letter.
> I was proposing that when virtio builds SIOV device it should start with #2.
> Because #1 is sort of handled with sr-iov or vdpa already.
> 
> #2 is more critical to get it right, after SIOV R2 spec is defined.


Can't we address both? E.g. use a different group type.


> 
> > For example,
> > consider compatibility. I know it looks like super important right now, and we
> > should make it possible, but it's just like with legacy interface - looked very far
> > away when we introduced non-transitional, to the point where an option in
> > linux to disable legacy talks about "flying cars", but the flying cars are nowhere
> > to be seen and, while we are not where everyone can just drop pre-1.0 bits, a
> > bunch of people are using e.g. vdpa without any legacy, express devices in
> > qemu also disabled legacy, and so on.
> > 
> > 
> > --
> > 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/

RE: [virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, August 2, 2023 5:21 PM

> 
> There's one thing to say about putting everything in one place, and that is that
> one can then find everything in one place.

When a device grows incrementally and each functionality also, 
grouping it per functionality gives better pace to keep things self-contained.

Think how messy a set command would be if all 3 functionality in one set command.

> For example, at the moment we actually do have a problem with cvq, and the
> problem is that it sets internal device state that is not observable (unlike original
> pre-1.0 config space which was writeable).
> 
Which part is not observable?

> 
> By the way there's a pci express ECN for relaxed ordering or something like this.
> I am yet to look at it, I wonder whether it can be used to avoid the issues we
> have with MMIO in a way that is easier to use than DMA.

Waiting to deliver new features on new PCI standard will drastically slow down virtio.
I don't think its good idea, but I would like to know about such ECN, if there is one.

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/

[virtio-comment] RE: proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, August 2, 2023 5:24 PM

> > 2. Build SIOV device ground up that does not need to have existing limitations
> of 1.x.
> >
> > The transport VQ proposal 4 patches had heavy inclination for #1 without
> mentioning the real objective in the cover letter.
> > I was proposing that when virtio builds SIOV device it should start with #2.
> > Because #1 is sort of handled with sr-iov or vdpa already.
> >
> > #2 is more critical to get it right, after SIOV R2 spec is defined.
> 
> 
> Can't we address both? E.g. use a different group type.

SIOV device is just one type, serving two kinds of commands one for supporting backward compat, second for newer interface.


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/

Re: [virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 11:56:11AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, August 2, 2023 5:21 PM
> 
> > 
> > There's one thing to say about putting everything in one place, and that is that
> > one can then find everything in one place.
> 
> When a device grows incrementally and each functionality also, 
> grouping it per functionality gives better pace to keep things self-contained.
> 
> Think how messy a set command would be if all 3 functionality in one set command.
> 
> > For example, at the moment we actually do have a problem with cvq, and the
> > problem is that it sets internal device state that is not observable (unlike original
> > pre-1.0 config space which was writeable).
> > 
> Which part is not observable?

e.g. rss config.

> > 
> > By the way there's a pci express ECN for relaxed ordering or something like this.
> > I am yet to look at it, I wonder whether it can be used to avoid the issues we
> > have with MMIO in a way that is easier to use than DMA.
> 
> Waiting to deliver new features on new PCI standard will drastically slow down virtio.

features? surely not. but when we are building a new transport then
we are building for the future, it takes years for a new
transport to be deployed.

> I don't think its good idea, but I would like to know about such ECN, if there is one.

I am talking about unordered IO (UIO) ECN.


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/

[virtio-comment] Re: proposal: use admin command (and aq) of the device to query config space

[Michael S. Tsirkin]

On Wed, Aug 02, 2023 at 11:58:31AM +0000, Parav Pandit wrote:
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Wednesday, August 2, 2023 5:24 PM
> 
> > > 2. Build SIOV device ground up that does not need to have existing limitations
> > of 1.x.
> > >
> > > The transport VQ proposal 4 patches had heavy inclination for #1 without
> > mentioning the real objective in the cover letter.
> > > I was proposing that when virtio builds SIOV device it should start with #2.
> > > Because #1 is sort of handled with sr-iov or vdpa already.
> > >
> > > #2 is more critical to get it right, after SIOV R2 spec is defined.
> > 
> > 
> > Can't we address both? E.g. use a different group type.
> 
> SIOV device is just one type, serving two kinds of commands one for supporting backward compat, second for newer interface.

Not sure about specifics but we can have two groups e.g. SIOV-compat and
SIOV-passthrough.


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/

[virtio-comment] Re: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Jason Wang]

On Wed, Aug 2, 2023 at 5:57 PM Parav Pandit <parav@nvidia.com> wrote:
>
>
>
> > From: Jason Wang <jasowang@redhat.com>
> > Sent: Wednesday, August 2, 2023 3:02 PM
> >
> > On Wed, Aug 2, 2023 at 5:07 PM Parav Pandit <parav@nvidia.com> wrote:
> > >
> > >
> > > > From: Jason Wang <jasowang@redhat.com>
> > > > Sent: Wednesday, August 2, 2023 2:23 PM
> > > >
> > > > On Tue, Aug 1, 2023 at 3:09 PM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > One line proposal:
> > > > > Let's use new admin command and admin q for all device types to
> > > > > query
> > > > device config space for new fields. (always).
> > > >
> > > > Before we mandate anything to admin command, we need to first invent
> > > > an admin command over MMIO interface otherwise it would always be an
> > > > issue for the nesting.
> > > >
> > > Nesting can be independent requirement in itself.'
> >
> > I don't understand here. If you tie new fields to the DMA interface, it basically
> > means nesting won't get any new features unless:
> >
> > 1) it's a PCI VF
> > 2) SR-IOV emulation is done
> > 3) admin virtqueue emulation is done
> >
> > If you want differ nesting devices from others, it would be a nightmare to
> > maintain.
> >
> New fields for sure are tied to the DMA interface.

This is different from what you've said above

"Let's use the new admin command and admin q for all device types to ..."

I'm simply replying to your proposal to tie new fields to the admin
command(queue).

> And nesting can also get just like how a VQ works in nested mode.

We should consider reuse the existing one like cvq or inventing
lightwight and self contained methods. Admin virtqueue doesn't fit,
admin command may but we need a MMIO interface for admin command.

>
> > > >
> > > > 1) device configuration space is transport independent, some
> > > > transport already use DMA to access the device configuration space
> > > You can say ccw instead of "some". :)
> >
> > Kind of but the transport vq proposal goes in the same way.
> >
> We debated many times that the wording transport vq is wrong as it is _not_ going to transport driver notifications.
> Anyway, there is nothing to discuss here. So focusing on main items below.
>
> > >
> > > > 2) device configuration space is not read only, we've already had
> > > > several examples of using it as write
> > > >
> > > It is even worse to have writable.
> >
> > Well, what I meant is that, it's not necessarily read only and not necessarily a
> > register interface.
> >
> I took PCI as being most common interface and took net, blk as devices who are experiencing high growth on features and device specific config space.
>
> This isn’t really a normative part of the spec. The key takeway to have it, for common things it is read only and its register.

As mentioned in the past, when developing spec, we should look for
what it can be.

My point is to stick to the device configuration space but invent DMA
interface to access them, then we are all fine.

>
> > >
> > > > > It is growing rapidly.
> > > > > Some devices may be even multi-functionality device in coming
> > > > > future such as
> > > > net + clock + rdma device.
> > > > > For a PCI transport implementing such ever-growing capabilities,
> > > > configuration is burdensome as plain registers.
> > > >
> > > > We've already fixed size VIRTIO_PCI_CAP_PCI_CFG. What's wrong with that?
> > > >
> > > The wrong part is: it is still and indirect and slow, sub-optimal register
> > interface.
> >
> > Do we really care about the performance here?
> When it comes to bulk data transfer, in range of few hundred bytes and looking for 5+ year period, than yes, reading using indirect register is slow.

What kind of configuration requires a few hundred bytes? We should not
duplicate the work of provisioning into device configuration space.

> And after all it is still indirect register which is does not have time variability part.
> So it is not fulfilling the requirement at all.
>
> > And if it is one of your major
> > concerns, it's better to explain it along with the "ever-growing" concern.
> >
> I thought it is clear that it is still a register, even slower than current one and still have same issues without it.
>
> > > VQ is decoupled from transport already.
> > > So, there is no flexibility broken.
> > > And yet you suggested transport dependent VIRTIO_PCI_CAP_PCI_CFG above
> > > making it further wrong. :)
> > >
> >
> > The context here is that you want to mandate any new fields to be DMA.
> > DMA is obviously transport specific. There are transports that don't use DMA at
> > all (e.g the shared memory).
> >
>
> VQ is surprisingly does the DMA without being transport specific.
> How net device mandate tx pkts via vq, how a console device mandate receive and transmit queue,
> How a crypto device mandate a control vq,
>
> What is proposed here is no different...
>
> Shared memory is not for bulk data transfer in virtio spec.
> We don’t see "shared memory" as transport in " Virtio Transport Options" section.

You can see some examples in the kernel drivers. The DMA mandating
excludes any shared memory proposal in the future.

>
> > > > > 3. A device must be able to choose, starting from which field
> > > > > driver must query such configuration via DMA interface. This field
> > > > > offset must be
> > > > greater than currently defined configuration field.
> >
> > [...]
> >
> > > > >
> > > > > d. Any other option?
> > > >
> > > > Transport virtqueue on top of admin virtqueue will address this seamlessly.
> > > >
> > > :)
> > >
> > > Don’t see why one would create few more objects on top of aq when aq or
> > cvq itself can fulfil the need.
> > > Can you please elaborate?
> >
> > If cvq can work, there's no need for any other methods.
> Cvq is not present for all the device, at the same time all devices are not experiencing high growth of config space either.

Adding cvq is much easier than inventing(duplicating) the work of a transport.

Thanks

> Hence the discussion here.
> Michael raised the concern to use some generic scheme for all the device previously, hence the aq.
>
> I think the practical approach is, whichever device has cvq, they should be able to use cvq for config+caps query+set.
> And rest can burn AQ to transport the same command.
>
> > If you care about admin
> > virtqueue then device configuration space is not the only thing that can be
> > "ever growing", common_cfg is another one.
> The idea is to do minimal bootstrap work from the common config space and switch to the queue.
> So common config shouldn’t be growing either other than minimal bootstrap functionality.
> Hence, common config also to be available via dma command.
>
> > Allowing them to be accessed via a
> > virtqueue (thus DMA in PCI transport) is basically the idea of the transport
> > virtqueue. Ling shan is rebasing transport virtqueue proposals on top of admin
> > commands, we can see then.
>
> Current proposal doesnt need yet another new transport vq with/without nesting.
> We just need to make choice as _always_admin command or efficiently use cvq when available.


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/

[virtio-comment] RE: [virtio] RE: [virtio-comment] proposal: use admin command (and aq) of the device to query config space

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, August 3, 2023 8:26 AM
> 
> On Wed, Aug 2, 2023 at 5:57 PM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Wednesday, August 2, 2023 3:02 PM
> > >
> > > On Wed, Aug 2, 2023 at 5:07 PM Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Wednesday, August 2, 2023 2:23 PM
> > > > >
> > > > > On Tue, Aug 1, 2023 at 3:09 PM Parav Pandit <parav@nvidia.com>
> wrote:
> > > > > >
> > > > > > One line proposal:
> > > > > > Let's use new admin command and admin q for all device types
> > > > > > to query
> > > > > device config space for new fields. (always).
> > > > >
> > > > > Before we mandate anything to admin command, we need to first
> > > > > invent an admin command over MMIO interface otherwise it would
> > > > > always be an issue for the nesting.
> > > > >
> > > > Nesting can be independent requirement in itself.'
> > >
> > > I don't understand here. If you tie new fields to the DMA interface,
> > > it basically means nesting won't get any new features unless:
> > >
> > > 1) it's a PCI VF
> > > 2) SR-IOV emulation is done
> > > 3) admin virtqueue emulation is done
> > >
> > > If you want differ nesting devices from others, it would be a
> > > nightmare to maintain.
> > >
> > New fields for sure are tied to the DMA interface.
> 
> This is different from what you've said above
> 
> "Let's use the new admin command and admin q for all device types to ..."
> 
> I'm simply replying to your proposal to tie new fields to the admin
> command(queue).
> 
New fields over the queuing interface.
Existing fields using config space register for backward compatibility.

Optionally, existing fields can be queried over queue as well, so if one is implementing new driver, it can always follow q over register.

> > And nesting can also get just like how a VQ works in nested mode.
> 
> We should consider reuse the existing one like cvq or inventing lightwight and
> self contained methods. 

true, in this proposal option_3 is the cvq.

> Admin virtqueue doesn't fit, admin command may but
> we need a MMIO interface for admin command.
>
Keeping nesting aside for a moment AQ fits but it has inefficiencies that I listed in the first email.
Inefficiency exists mainly for those devices which already has cvq.

so a new device or existing device without cvq, when the need arise can add cvq to achieve what net, gpu, crypo will do now.


> >
> > > > >
> > > > > 1) device configuration space is transport independent, some
> > > > > transport already use DMA to access the device configuration
> > > > > space
> > > > You can say ccw instead of "some". :)
> > >
> > > Kind of but the transport vq proposal goes in the same way.
> > >
> > We debated many times that the wording transport vq is wrong as it is _not_
> going to transport driver notifications.
> > Anyway, there is nothing to discuss here. So focusing on main items below.
> >
> > > >
> > > > > 2) device configuration space is not read only, we've already
> > > > > had several examples of using it as write
> > > > >
> > > > It is even worse to have writable.
> > >
> > > Well, what I meant is that, it's not necessarily read only and not
> > > necessarily a register interface.
> > >
> > I took PCI as being most common interface and took net, blk as devices who
> are experiencing high growth on features and device specific config space.
> >
> > This isn’t really a normative part of the spec. The key takeway to have it, for
> common things it is read only and its register.
> 
> As mentioned in the past, when developing spec, we should look for what it can
> be.
> 
> My point is to stick to the device configuration space but invent DMA interface
> to access them, then we are all fine.
> 
Yes, option_3 using cvq that utilize the dma interface.
Option_2 using aq.

> >
> > > >
> > > > > > It is growing rapidly.
> > > > > > Some devices may be even multi-functionality device in coming
> > > > > > future such as
> > > > > net + clock + rdma device.
> > > > > > For a PCI transport implementing such ever-growing
> > > > > > capabilities,
> > > > > configuration is burdensome as plain registers.
> > > > >
> > > > > We've already fixed size VIRTIO_PCI_CAP_PCI_CFG. What's wrong with
> that?
> > > > >
> > > > The wrong part is: it is still and indirect and slow, sub-optimal
> > > > register
> > > interface.
> > >
> > > Do we really care about the performance here?
> > When it comes to bulk data transfer, in range of few hundred bytes and
> looking for 5+ year period, than yes, reading using indirect register is slow.
> 
> What kind of configuration requires a few hundred bytes? We should not
> duplicate the work of provisioning into device configuration space.
> 
New steering feature is taking up 16 bytes just in the requirements phase. When we do design, we will find more need.
Counters bitmap..
Timestamping need similar several tens of bytes.
Many requirements that are being worked, sum up to 100 bytes.

> > And after all it is still indirect register which is does not have time variability
> part.
> > So it is not fulfilling the requirement at all.
> >
> > > And if it is one of your major
> > > concerns, it's better to explain it along with the "ever-growing" concern.
> > >
> > I thought it is clear that it is still a register, even slower than current one and
> still have same issues without it.
> >
> > > > VQ is decoupled from transport already.
> > > > So, there is no flexibility broken.
> > > > And yet you suggested transport dependent VIRTIO_PCI_CAP_PCI_CFG
> > > > above making it further wrong. :)
> > > >
> > >
> > > The context here is that you want to mandate any new fields to be DMA.
> > > DMA is obviously transport specific. There are transports that don't
> > > use DMA at all (e.g the shared memory).
> > >
> >
> > VQ is surprisingly does the DMA without being transport specific.
> > How net device mandate tx pkts via vq, how a console device mandate
> > receive and transmit queue, How a crypto device mandate a control vq,
> >
> > What is proposed here is no different...
> >
> > Shared memory is not for bulk data transfer in virtio spec.
> > We don’t see "shared memory" as transport in " Virtio Transport Options"
> section.
> 
> You can see some examples in the kernel drivers. The DMA mandating excludes
> any shared memory proposal in the future.
> 
Not really, when one comes up with shared memory, a new feature bit can expose.

> >
> > > > > > 3. A device must be able to choose, starting from which field
> > > > > > driver must query such configuration via DMA interface. This
> > > > > > field offset must be
> > > > > greater than currently defined configuration field.
> > >
> > > [...]
> > >
> > > > > >
> > > > > > d. Any other option?
> > > > >
> > > > > Transport virtqueue on top of admin virtqueue will address this
> seamlessly.
> > > > >
> > > > :)
> > > >
> > > > Don’t see why one would create few more objects on top of aq when
> > > > aq or
> > > cvq itself can fulfil the need.
> > > > Can you please elaborate?
> > >
> > > If cvq can work, there's no need for any other methods.
> > Cvq is not present for all the device, at the same time all devices are not
> experiencing high growth of config space either.
> 
> Adding cvq is much easier than inventing(duplicating) the work of a transport.
> 
+1.