[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [virtio-dev] [PATCH v13 03/11] content: Rename confusing queue_notify_data and vqn names
> From: Halil Pasic <pasic@linux.ibm.com> > 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 > <PH0PR12MB54812C754ED9063E34E06E1BDC9A9@PH0PR12MB5481.namprd > 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 -\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?
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]