[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]
Subject: Re: [virtio-comment] Re: [PATCH v6 4/5] Add admin_queue_index register to PCI common configuration structure
On Thu, Aug 04, 2022 at 03:01:44AM +0300, Max Gurtovoy wrote: > > On 8/1/2022 9:13 AM, Michael S. Tsirkin wrote: > > On Mon, Aug 01, 2022 at 03:11:58AM +0300, Max Gurtovoy wrote: > > > On 8/1/2022 12:03 AM, Michael S. Tsirkin wrote: > > > > On Sun, Jul 31, 2022 at 06:43:53PM +0300, Max Gurtovoy wrote: > > > > > This new register will be used for querying the index of the admin > > > > > virtqueue of a virtio device. To configure, reset or enable the admin > > > > > virtqueue, the driver should follow existing queue configuration/setup > > > > > sequence. > > > > > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com> > > > > > Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com> > > > > Can you please at least add text to MMIO and CCW that drivers and > > > > devices must not negotiate the new feature bit? Will help avoid confusion. > > > why this is needed ? who will create a device and expose bits that it > > > doesn't know how to implement. > > Any multi-transport implementation actually. > > For example, on a Linux guest the default is to add a feature bit to all > > transports. Extra care is needed to actually only add it > > to the PCI transport. With qemu, same applies to features in > > include/hw/virtio/virtio.h. > > I don't know how a reasonable driver will want to negotiate feature bits > that a device didn't expose. As a result of a bug. > Please suggest whatever you want me to add and where. > > I don't mind adding this but I don't want to waste cycle of review on that > so I need exact guideline and not hints please. Sure. So "The driver MUST NOT negotiate VIRTIO_F_ADMIN_QUEUE. This is because there is as yet no way for the driver to discover the index of the admin queue." in Device Initialization chapter. And similarly add a device normative: "The device MUST NOT offer VIRTIO_F_ADMIN_QUEUE. This is because there is as yet no way to device to expose the index of the admin queue." > > > > > And if I'll add it, we might forget to remove it later on. > > When we add the implementation I'm reasonably sure we won't forget to > > remove text saying it's not implemented. > > Ok. > > > > > > > > > --- > > > > > content.tex | 10 ++++++++++ > > > > > 1 file changed, 10 insertions(+) > > > > > > > > > > diff --git a/content.tex b/content.tex > > > > > index c15423e..5fda1a0 100644 > > > > > --- a/content.tex > > > > > +++ b/content.tex > > > > > @@ -904,6 +904,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport > > > > > le64 queue_device; /* read-write */ > > > > > le16 queue_notify_data; /* read-only for driver */ > > > > > le16 queue_reset; /* read-write */ > > > > > + > > > > > + /* About admin virtqueue. */ > > > > > + le16 admin_queue_index; /* read-only for driver */ > > > > > }; > > > > > \end{lstlisting} > > > > > @@ -989,6 +992,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport > > > > > This field exists only if VIRTIO_F_RING_RESET has been > > > > > negotiated. (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > > > > +\item[\field{admin_queue_index}] > > > > > + The device uses this to report the index of the admin virtqueue. > > > > > + This field always exists. Its value is valid only if VIRTIO_F_ADMIN_VQ has been negotiated. > > > > > + > > > > > \end{description} > > > > > \devicenormative{\paragraph}{Common configuration structure layout}{Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Common configuration structure layout} > > > > we have a mess with this exists versus valid. I think exists is the same > > > the bigest mess that we have is for things like ring_reset that are > > > optionally exist according to bit negotiation. > > I think this optionally exist does not mean much, just that > > it should not be accessed > > If I understand the word "exist" translation correctly, it doesn't mean what > you said about "shouldn't be accessed". > > > > about valid - the driver shouldn't even look on registers that it didn't > > > negotiated it's feature bits. There is no reason to do so. > > > So yes, there is a mess but not in our proposal. > > Right. I propose removing "This field always exists." just > > say that it's valid with VIRTIO_F_ADMIN_VQ. > > Why removing ? does it exist always or not ? It doesn't. For example existing devices do not have it. > > > > > > is valid personally. Do others object if we say same as for reset here? > > > > Not a big deal either way, we need to clean this up later. > > > > > > > > > @@ -1075,6 +1082,9 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport > > > > > were used before the queue reset. > > > > > (see \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}). > > > > > +For configuring the admin virtqueue, the driver MUST use the value of \field{admin_queue_index}. > > > > > +For more details on virtqueue configuration see section \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI-specific Initialization And Device Operation / Device Initialization / Virtqueue Configuration}. > > > > > + > > > > > \subsubsection{Notification structure layout}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / PCI Device Layout / Notification capability} > > > > > The notification location is found using the VIRTIO_PCI_CAP_NOTIFY_CFG > > > > > -- > > > > > 2.21.0 > > > > 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.oasis-open.org%2Farchives%2Fvirtio-comment%2F&data=05%7C01%7Cmgurtovoy%40nvidia.com%7C46caf90d77cb4e2e7d1b08da7384fbe5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637949312238253496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=72o5SFbI1Ai5twIQXF0Onn2TTorpuIlWoqp46tpUgrQ%3D&reserved=0 > > Feedback License: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fwho%2Fipr%2Ffeedback_license.pdf&data=05%7C01%7Cmgurtovoy%40nvidia.com%7C46caf90d77cb4e2e7d1b08da7384fbe5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637949312238253496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=7%2FdpXHFFpA7j97vIqLFXfaVQMj1FmDX2Ldwwzytpuuw%3D&reserved=0 > > List Guidelines: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fpolicies-guidelines%2Fmailing-lists&data=05%7C01%7Cmgurtovoy%40nvidia.com%7C46caf90d77cb4e2e7d1b08da7384fbe5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637949312238253496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lN2X%2BC%2BoHvd90YL%2BSf3FSleiNJYZnfTqRWSdDpK2wAc%3D&reserved=0 > > Committee: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fcommittees%2Fvirtio%2F&data=05%7C01%7Cmgurtovoy%40nvidia.com%7C46caf90d77cb4e2e7d1b08da7384fbe5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637949312238253496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=5FFookod06loAfQ8uGp3HmeuEUdP2lurXWtPNOnCtNY%3D&reserved=0 > > Join OASIS: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.oasis-open.org%2Fjoin%2F&data=05%7C01%7Cmgurtovoy%40nvidia.com%7C46caf90d77cb4e2e7d1b08da7384fbe5%7C43083d15727340c1b7db39efd9ccc17a%7C0%7C0%7C637949312238253496%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=1OJe%2BI1%2FsWYMIaM0PIZjMKmcC%2FpGAoOkHr%2Bi4E4AJOE%3D&reserved=0 > >
[Date Prev] | [Thread Prev] | [Thread Next] | [Date Next] -- [Date Index] | [Thread Index] | [List Home]