Re: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
On Tue, Apr 18 2023, Halil Pasic wrote:
> On Mon, 17 Apr 2023 20:53:32 +
> Parav Pandit wrote:
>
>> > From: Halil Pasic
>> > Sent: Sunday, April 16, 2023 11:42 PM
> [..]
>> >
>> > > @@ -1053,9 +1059,9 @@ \subsubsection{Available Buffer
>> > > Notifications}\label{sec:Virtio Transport Option If
>> > VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
>> > > \begin{itemize}
>> > > \item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the
>> > > driver MUST use the -\field{queue_notify_data} value instead of the
>> > > virtqueue
>> > index.
>> > > +\field{queue_notify_id} value instead of the virtqueue index.
>> > > \item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
>> > > MUST set the -\field{vqn} field to the \field{queue_notify_data} value.
>> > > +\field{vq_notify_id} field to the \field{queue_notify_id} value.
>> >
>> > Seems you did not catch these with the s/_id/_config_data/. By doing the
>> > replace one would get queue_notify_config_data, vq_notify_config_data and
>> > queue_notify_config_data respectively.
>> >
>> > But that still does not seem right, and the right answer depends on to
>> > what do
>> > we rename \field{vqn}.
>> >
>> vqn -> vq_notif_config_data in the notification structure.
>
> I think I got what you mean. I intend to double check it
> in v14.
>
>>
>> virtio_pci_common_cfg.queue_notify_data -> queue_notify_config_data.
>>
>
> I have no strong opinion about this one.
>
>> Better to rename
>>
>> VIRTIO_F_NOTIF_CONFIG_DATA to VIRTIO_F_NOTIFY_CONFIG_DATA.
>> Adding "Y" is not going to make it that long. Field names will have some
>> consistency.
>> WDYT?
>
> No strong opinion on this either. I agree with Michael that "NOTIF"
> likely stands for "notification", but I also see your point: having
> "notify" in the field names and "NOTIF" in the feature name isn't great
> either.
>
> IMHO VIRTIO_F_NOTIFY_CONFIG_DATA should remain aligned with
> VIRTIO_F_NOTIFICATION_DATA in one way or another. Thus way out would be
> to do something like:
> virtio_pci_common_cfg.queue_notify_data -> queue_notif_config_data
>
> I think as editors Connie and Michael should have the most say in this
> though.
Using "notif" throughout would probably be most consistent.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] RE: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
On Tue, Apr 18 2023, Parav Pandit wrote: >> From: Halil Pasic >> Sent: Tuesday, April 18, 2023 7:40 AM >> >> IMHO VIRTIO_F_NOTIFY_CONFIG_DATA should remain aligned with >> VIRTIO_F_NOTIFICATION_DATA in one way or another. Thus way out would be >> to do something like: >> virtio_pci_common_cfg.queue_notify_data -> queue_notif_config_data >> >> I think as editors Connie and Michael should have the most say in this >> though. > > Are you ok with above name? I still need to read through the whole thing. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
> From: Michael S. Tsirkin > Sent: Tuesday, April 18, 2023 7:43 AM > > IMHO VIRTIO_F_NOTIFY_CONFIG_DATA should remain aligned with > > VIRTIO_F_NOTIFICATION_DATA in one way or another. Thus way out would > > be to do something like: > > virtio_pci_common_cfg.queue_notify_data -> queue_notif_config_data > > > > I think as editors Connie and Michael should have the most say in this > > though. > > > > Regards, > > Halil > > Fine by me. Ok. read it now. Please ignore my previous email question. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
> From: Halil Pasic > Sent: Tuesday, April 18, 2023 7:40 AM > > IMHO VIRTIO_F_NOTIFY_CONFIG_DATA should remain aligned with > VIRTIO_F_NOTIFICATION_DATA in one way or another. Thus way out would be > to do something like: > virtio_pci_common_cfg.queue_notify_data -> queue_notif_config_data > > I think as editors Connie and Michael should have the most say in this though. Are you ok with above name? - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
On Mon, 17 Apr 2023 20:53:32 +
Parav Pandit wrote:
> > From: Halil Pasic
> > Sent: Sunday, April 16, 2023 11:42 PM
[..]
> >
> > > @@ -1053,9 +1059,9 @@ \subsubsection{Available Buffer
> > > Notifications}\label{sec:Virtio Transport Option If
> > VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
> > > \begin{itemize}
> > > \item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the
> > > driver MUST use the -\field{queue_notify_data} value instead of the
> > > virtqueue
> > index.
> > > +\field{queue_notify_id} value instead of the virtqueue index.
> > > \item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
> > > MUST set the -\field{vqn} field to the \field{queue_notify_data} value.
> > > +\field{vq_notify_id} field to the \field{queue_notify_id} value.
> >
> > Seems you did not catch these with the s/_id/_config_data/. By doing the
> > replace one would get queue_notify_config_data, vq_notify_config_data and
> > queue_notify_config_data respectively.
> >
> > But that still does not seem right, and the right answer depends on to what
> > do
> > we rename \field{vqn}.
> >
> vqn -> vq_notif_config_data in the notification structure.
I think I got what you mean. I intend to double check it
in v14.
>
> virtio_pci_common_cfg.queue_notify_data -> queue_notify_config_data.
>
I have no strong opinion about this one.
> Better to rename
>
> VIRTIO_F_NOTIF_CONFIG_DATA to VIRTIO_F_NOTIFY_CONFIG_DATA.
> Adding "Y" is not going to make it that long. Field names will have some
> consistency.
> WDYT?
No strong opinion on this either. I agree with Michael that "NOTIF"
likely stands for "notification", but I also see your point: having
"notify" in the field names and "NOTIF" in the feature name isn't great
either.
IMHO VIRTIO_F_NOTIFY_CONFIG_DATA should remain aligned with
VIRTIO_F_NOTIFICATION_DATA in one way or another. Thus way out would be
to do something like:
virtio_pci_common_cfg.queue_notify_data -> queue_notif_config_data
I think as editors Connie and Michael should have the most say in this
though.
Regards,
Halil
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
Re: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
On Mon, Apr 17, 2023 at 08:53:32PM +, Parav Pandit wrote:
> > > @@ -1053,9 +1059,9 @@ \subsubsection{Available Buffer
> > > Notifications}\label{sec:Virtio Transport Option If
> > VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
> > > \begin{itemize}
> > > \item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the
> > > driver MUST use the -\field{queue_notify_data} value instead of the
> > > virtqueue
> > index.
> > > +\field{queue_notify_id} value instead of the virtqueue index.
> > > \item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
> > > MUST set the -\field{vqn} field to the \field{queue_notify_data} value.
> > > +\field{vq_notify_id} field to the \field{queue_notify_id} value.
> >
> > Seems you did not catch these with the s/_id/_config_data/. By doing the
> > replace one would get queue_notify_config_data, vq_notify_config_data and
> > queue_notify_config_data respectively.
> >
> > But that still does not seem right, and the right answer depends on to what
> > do
> > we rename \field{vqn}.
> >
> vqn -> vq_notif_config_data in the notification structure.
>
> virtio_pci_common_cfg.queue_notify_data -> queue_notify_config_data.
>
> Better to rename
>
> VIRTIO_F_NOTIF_CONFIG_DATA to VIRTIO_F_NOTIFY_CONFIG_DATA.
> Adding "Y" is not going to make it that long. Field names will have some
> consistency.
> WDYT?
I think this NOTIF stands for "notification", not "notify".
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
RE: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
> From: Halil Pasic
> Sent: Sunday, April 16, 2023 11:42 PM
> > diff --git a/notifications-le.c b/notifications-le.c index
> > fe51267..f73c6a5 100644
> > --- a/notifications-le.c
> > +++ b/notifications-le.c
> > @@ -1,5 +1,5 @@
> > le32 {
> > - vqn : 16;
> > + vq_config_data: 16; /* previously known as vqn */
>
> Is this where the union was supposed to go? I.e.
> something like:
>
> union {
> le16 vq_config_data;
> le16 vq_index;
> } vq_index_config_data;
>
> (You said in the v12 disussion in MID
> 12.prod.outlook.com>
> that you are going to change vqn to the union of vq_config_data and vq_index,
> although vq_notif_config_data might be preferable).
>
Ah my bad. I again got confused when writing the patch that this structure is
only for notify_config_data.
It is not.
I will fix the union.
I will let be unnamed.
I will rename vqn to vq_notif_config_data.
> Or did you mean to do
>
> + vq_index_config_data: 16; /* previously known as vqn */
>
> Of course, this has an impact on the rest of the text...
>
Union is more readable. Will keep union.
> > next_off : 15;
> > next_wrap : 1;
> > };
> > diff --git a/transport-pci.tex b/transport-pci.tex index
> > 5d98467..53c8ee6 100644
> > --- a/transport-pci.tex
> > +++ b/transport-pci.tex
> > @@ -319,7 +319,7 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> > le64 queue_desc;/* read-write */
> > le64 queue_driver; /* read-write */
> > le64 queue_device; /* read-write */
> > -le16 queue_notify_data; /* read-only for driver */
> > +le16 queue_notify_config_data; /* read-only for driver */
> > le16 queue_reset; /* read-write */
> > };
> > \end{lstlisting}
> > @@ -388,17 +388,21 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport \item[\field{queue_device}]
> > The driver writes the physical address of Device Area here. See
> > section
> \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
> >
> > -\item[\field{queue_notify_data}]
> > +\item[\field{queue_notify_config_data}]
> > This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been
> negotiated.
> > -The driver will use this value to put it in the 'virtqueue number'
> > field
> > -in the available buffer notification structure.
> > +The driver will use this value when driver sends available buffer
> > +notification to the device.
> > See section \ref{sec:Virtio Transport Options / Virtio Over PCI
> > Bus / PCI-
> specific Initialization And Device Operation / Available Buffer
> Notifications}.
> > \begin{note}
> > This field provides the device with flexibility to determine how
> virtqueues
> > will be referred to in available buffer notifications.
> > -In a trivial case the device can set
> > \field{queue_notify_data}=vqn. Some
> devices
> > -may benefit from providing another value, for example an internal
> virtqueue
> > -identifier, or an internal offset related to the virtqueue number.
> > +In a trivial case the device can set
> > \field{queue_notify_config_data} to
> > +virtqueue index. Some devices may benefit from providing another
> value,
> > +for example an internal virtqueue identifier, or an internal offset
> > +related to the virtqueue index.
> > +\end{note}
> > +\begin{note}
> > +This field is previously known as queue_notify_data.
> > \end{note}
> >
> > \item[\field{queue_reset}]
> > @@ -468,7 +472,9 @@ \subsubsection{Common configuration structure
> > layout}\label{sec:Virtio Transport
> >
> > \drivernormative{\paragraph}{Common configuration structure
> > layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device
> > Layout / Common configuration structure layout}
> >
> > -The driver MUST NOT write to \field{device_feature}, \field{num_queues},
> \field{config_generation}, \field{queue_notify_off} or
> \field{queue_notify_data}.
> > +The driver MUST NOT write to \field{device_feature},
> > +\field{num_queues}, \field{config_generation},
> > +\field{queue_notify_off} or \field{queue_notify_config_data}.
> >
> > If VIRTIO_F_RING_PACKED has been negotiated, the driver MUST NOT
> > write the value 0 to \field{queue_size}.
>
> > @@ -1053,9 +1059,9 @@ \subsubsection{Available Buffer
> > Notifications}\label{sec:Virtio Transport Option If
> VIRTIO_F_NOTIF_CONFIG_DATA has been negotiated:
> > \begin{itemize}
> > \item If VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the
> > driver MUST use the -\field{queue_notify_data} value instead of the
> > virtqueue
> index.
> > +\field{queue_notify_id} value instead of the virtqueue index.
> > \item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver
> > MUST set the
Re: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
On Tue, 11 Apr 2023 22:23:33 +0300
Parav Pandit wrote:
> Currently queue_notify_data register indicates the device
> internal queue notification content. This register is
> meaningful only when feature bit VIRTIO_F_NOTIF_CONFIG_DATA is
> negotiated.
>
> However, above register name often get confusing association with
> very similar feature bit VIRTIO_F_NOTIFICATION_DATA.
>
> When VIRTIO_F_NOTIFICATION_DATA feature bit is negotiated,
> notification really involves sending additional queue progress
> related information (not queue identifier or index).
>
> Hence
> 1. to avoid any misunderstanding and association of
> queue_notify_data with similar name VIRTIO_F_NOTIFICATION_DATA,
>
> and
> 2. to reflect that queue_notify_data is the actual device
> internal virtqueue identifier/index/data/cookie,
>
> a. rename queue_notify_data to queue_notify_config_data.
> queue_notify_config_data.
>
> b. rename ambiguous vqn to a union of vq_index and vq_config_data
>
> c. The driver notification section assumes that queue notification contains
> vq index always. CONFIG_DATA feature bit introduction missed to
> update the driver notification section. Hence, correct it.
>
> Signed-off-by: Parav Pandit
>
> ---
> changelog:
> v12->v13:
> - replace _id with _config_data
> - dropped vq identifier
> - dropped the idea of union as description is for config data feature
> v11->v12:
> - new patch
> ---
> content.tex| 11 ---
> notifications-be.c | 2 +-
> notifications-le.c | 2 +-
> transport-pci.tex | 26 --
> 4 files changed, 26 insertions(+), 15 deletions(-)
>
> diff --git a/content.tex b/content.tex
> index e79d722..601c069 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -404,8 +404,12 @@ \section{Driver Notifications} \label{sec:Basic
> Facilities of a Virtio Device /
> notification to the device.
>
> When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
> -this notification involves sending the
> -virtqueue index to the device (method depending on the transport).
> +this notification contains either virtqueue
> +index if VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated or device
> +supplied virtqueue notification data if
> +VIRTIO_F_NOTIF_CONFIG_DATA is negotiated.
> +
> +A method to supply such virtqueue notification data is transport specific.
>
> However, some devices benefit from the ability to find out the
> amount of available data in the queue without accessing the virtqueue in
> memory:
> @@ -416,7 +420,8 @@ \section{Driver Notifications} \label{sec:Basic
> Facilities of a Virtio Device /
> the following information:
>
> \begin{description}
> -\item [vqn] VQ number to be notified.
> +\item [vq_index_config_data] either virtqueue index or device supplied
> + queue notification config data corresponding to a virtqueue.
> \item [next_off] Offset
>within the ring where the next available ring entry
>will be written.
> diff --git a/notifications-be.c b/notifications-be.c
> index 5be947e..02f0624 100644
> --- a/notifications-be.c
> +++ b/notifications-be.c
> @@ -1,5 +1,5 @@
> be32 {
> - vqn : 16;
> + vq_config_data: 16; /* previously known as vqn */
> next_off : 15;
> next_wrap : 1;
> };
> diff --git a/notifications-le.c b/notifications-le.c
> index fe51267..f73c6a5 100644
> --- a/notifications-le.c
> +++ b/notifications-le.c
> @@ -1,5 +1,5 @@
> le32 {
> - vqn : 16;
> + vq_config_data: 16; /* previously known as vqn */
Is this where the union was supposed to go? I.e.
something like:
union {
le16 vq_config_data;
le16 vq_index;
} vq_index_config_data;
(You said in the v12 disussion in MID
that you are going to change vqn to the union of vq_config_data and
vq_index, although vq_notif_config_data might be preferable).
Or did you mean to do
+ vq_index_config_data: 16; /* previously known as vqn */
Of course, this has an impact on the rest of the text...
> next_off : 15;
> next_wrap : 1;
> };
> diff --git a/transport-pci.tex b/transport-pci.tex
> index 5d98467..53c8ee6 100644
> --- a/transport-pci.tex
> +++ b/transport-pci.tex
> @@ -319,7 +319,7 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> le64 queue_desc;/* read-write */
> le64 queue_driver; /* read-write */
> le64 queue_device; /* read-write */
> -le16 queue_notify_data; /* read-only for driver */
> +le16 queue_notify_config_data; /* read-only for driver */
> le16 queue_reset; /* read-write */
> };
> \end{lstlisting}
> @@ -388,17 +388,21 @@ \subsubsection{Common configuration structure
> layout}\label{sec:Virtio Transport
> \item[\field{queue_device}]
> The driver writes the physical address of Device Area here. See
> section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
>
>
[virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
Currently queue_notify_data register indicates the device
internal queue notification content. This register is
meaningful only when feature bit VIRTIO_F_NOTIF_CONFIG_DATA is
negotiated.
However, above register name often get confusing association with
very similar feature bit VIRTIO_F_NOTIFICATION_DATA.
When VIRTIO_F_NOTIFICATION_DATA feature bit is negotiated,
notification really involves sending additional queue progress
related information (not queue identifier or index).
Hence
1. to avoid any misunderstanding and association of
queue_notify_data with similar name VIRTIO_F_NOTIFICATION_DATA,
and
2. to reflect that queue_notify_data is the actual device
internal virtqueue identifier/index/data/cookie,
a. rename queue_notify_data to queue_notify_config_data.
queue_notify_config_data.
b. rename ambiguous vqn to a union of vq_index and vq_config_data
c. The driver notification section assumes that queue notification contains
vq index always. CONFIG_DATA feature bit introduction missed to
update the driver notification section. Hence, correct it.
Signed-off-by: Parav Pandit
---
changelog:
v12->v13:
- replace _id with _config_data
- dropped vq identifier
- dropped the idea of union as description is for config data feature
v11->v12:
- new patch
---
content.tex| 11 ---
notifications-be.c | 2 +-
notifications-le.c | 2 +-
transport-pci.tex | 26 --
4 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/content.tex b/content.tex
index e79d722..601c069 100644
--- a/content.tex
+++ b/content.tex
@@ -404,8 +404,12 @@ \section{Driver Notifications} \label{sec:Basic Facilities
of a Virtio Device /
notification to the device.
When VIRTIO_F_NOTIFICATION_DATA has not been negotiated,
-this notification involves sending the
-virtqueue index to the device (method depending on the transport).
+this notification contains either virtqueue
+index if VIRTIO_F_NOTIF_CONFIG_DATA is not negotiated or device
+supplied virtqueue notification data if
+VIRTIO_F_NOTIF_CONFIG_DATA is negotiated.
+
+A method to supply such virtqueue notification data is transport specific.
However, some devices benefit from the ability to find out the
amount of available data in the queue without accessing the virtqueue in
memory:
@@ -416,7 +420,8 @@ \section{Driver Notifications} \label{sec:Basic Facilities
of a Virtio Device /
the following information:
\begin{description}
-\item [vqn] VQ number to be notified.
+\item [vq_index_config_data] either virtqueue index or device supplied
+ queue notification config data corresponding to a virtqueue.
\item [next_off] Offset
within the ring where the next available ring entry
will be written.
diff --git a/notifications-be.c b/notifications-be.c
index 5be947e..02f0624 100644
--- a/notifications-be.c
+++ b/notifications-be.c
@@ -1,5 +1,5 @@
be32 {
- vqn : 16;
+ vq_config_data: 16; /* previously known as vqn */
next_off : 15;
next_wrap : 1;
};
diff --git a/notifications-le.c b/notifications-le.c
index fe51267..f73c6a5 100644
--- a/notifications-le.c
+++ b/notifications-le.c
@@ -1,5 +1,5 @@
le32 {
- vqn : 16;
+ vq_config_data: 16; /* previously known as vqn */
next_off : 15;
next_wrap : 1;
};
diff --git a/transport-pci.tex b/transport-pci.tex
index 5d98467..53c8ee6 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -319,7 +319,7 @@ \subsubsection{Common configuration structure
layout}\label{sec:Virtio Transport
le64 queue_desc;/* read-write */
le64 queue_driver; /* read-write */
le64 queue_device; /* read-write */
-le16 queue_notify_data; /* read-only for driver */
+le16 queue_notify_config_data; /* read-only for driver */
le16 queue_reset; /* read-write */
};
\end{lstlisting}
@@ -388,17 +388,21 @@ \subsubsection{Common configuration structure
layout}\label{sec:Virtio Transport
\item[\field{queue_device}]
The driver writes the physical address of Device Area here. See
section \ref{sec:Basic Facilities of a Virtio Device / Virtqueues}.
-\item[\field{queue_notify_data}]
+\item[\field{queue_notify_config_data}]
This field exists only if VIRTIO_F_NOTIF_CONFIG_DATA has been
negotiated.
-The driver will use this value to put it in the 'virtqueue number'
field
-in the available buffer notification structure.
+The driver will use this value when driver sends available buffer
+notification to the device.
See section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus /
PCI-specific Initialization And Device Operation / Available Buffer
Notifications}.
\begin{note}
This field provides the device with flexibility to determine how
virtqueues
will be referred to in available buffer notifications.
-In a trivial case the device can set
