[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: RE: [PATCH v14 03/11] content: Rename confusing queue_notify_data and vqn names
> From: Halil Pasic <pasic@linux.ibm.com> > Sent: Friday, April 21, 2023 6:38 PM > > > > \begin{description} > > -\item [vqn] VQ number to be notified. > > +\item [vq_index_config_data] Either virtqueue index or device > > +supplied > > Apparently this our name for either index or notification config data, but I don't > think we ever use it. > Right because this is generic description without the field data type and their endianness and transport specific information. So, it uses an abstract term. > I guess, there is a two to think about it: > * Notifications are mostly transport specific. In particular what > data structures are used for the virtqueue notifications and how > they are passed, that is entirely transport specific. So the 'layout' > and the normative statements describing the details should be in the > transport sections. Right. This addition did not add any normative. They are in transport section as you say. > * The facility bits and the capabilities are basically common. I.e. if > MMIO or/and Channel I/O were to ever support VIRTIO_F_NOTIF_CONFIG > data > (by providing means for transporting the virtqueue notification config > data from the device to the driver), what you call the notification > value would be either virtqueue index or this notification config data. > > You are taking the first approach here, and I'm fine with that. IMHO before we > had the second approach implemented with vqn. I'm fine with taking the first > approach as well, but your commit message says "rename ambiguous vqn to a > union of vq_index and vq_config_data" which for me implies you actually > intended to take the second approach. > Those are renamed where the actual vqn is defined in the structure like below. In basic description section, it is just describing the functionality. > > @@ -1,5 +1,5 @@ > > le32 { > > - vqn : 16; > > + vq_index: 16; /* previously known as vqn */ > > next_off : 15; > > next_wrap : 1; > > }; > > diff --git a/notifications-pci-le.c b/notifications-pci-le.c new file > > mode 100644 index 0000000..ef60d15 > > --- /dev/null > > +++ b/notifications-pci-le.c > > @@ -0,0 +1,8 @@ > > +le32 { > > + union { > > + vq_index: 16; /* Used if VIRTIO_F_NOTIF_CONFIG_DATA not > negotiated */ > > + vq_notif_config_data: 16; /* Used if > VIRTIO_F_NOTIF_CONFIG_DATA negotiated */ > > + }; > > + next_off : 15; > > + next_wrap : 1; > > +}; > > diff --git a/transport-pci.tex b/transport-pci.tex index > > 5d98467..04a9a80 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_notif_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_notif_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_notif_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_notif_config_data}. > > > > If VIRTIO_F_RING_PACKED has been negotiated, the driver MUST NOT > > write the value 0 to \field{queue_size}. > > @@ -1035,13 +1041,22 @@ \subsubsection{Available Buffer > > Notifications}\label{sec:Virtio Transport Option > > > > When VIRTIO_F_NOTIFICATION_DATA has not been negotiated, the driver > > sends an available buffer notification to the device by writing -the > > 16-bit virtqueue index -of this virtqueue to the Queue Notify address. > > +only the 16-bit notification value to the Queue Notify address of the > > > You introduce the term notification value. Which basically means virtqueue > index or virtqueue notification config data. Right and it is captured in the union fields. > Which is basically what is defined > where you define vq_index_config_data (the latter is never ever used again). > vq_index_config_data is abstract term without any bit fields, width, endinenss and without any attachment to transport details. > > +virtqueue. A notification value depends on the negotiation of > > +VIRTIO_F_NOTIF_CONFIG_DATA. > > > > -When VIRTIO_F_NOTIFICATION_DATA has been negotiated, -the driver > > sends an available buffer notification to the device by writing -the > > following 32-bit value to the Queue Notify address: > > -\lstinputlisting{notifications-le.c} > > +\item If VIRTIO_F_NOTIFICATION_DATA has been negotiated, the driver > > +sends an > > The leading \item is probably not intended. Breaks the build and makes no > sense. Yes. it was not intended. Good catch. Fixing in v15.
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]