Re: [PATCH 2/2] virtio: pci support virtqueue selective enabling

[Eugenio Perez Martin <eperezma@redhat.com>]

On Wed, Jun 7, 2023 at 11:03âAM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jun 07, 2023 at 09:37:00AM +0200, Eugenio Perez Martin wrote:
> > On Tue, Jun 6, 2023 at 9:09âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Jun 06, 2023 at 07:55:11PM +0200, Eugenio PÃrez wrote:
> > > > Reusing virtqueue reset method to enabling a vq.
> > > >
> > > > Signed-off-by: Eugenio PÃrez <eperezma@redhat.com>
> > > > ---
> > > >  transport-pci.tex | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/transport-pci.tex b/transport-pci.tex
> > > > index a5c6719..04c0429 100644
> > > > --- a/transport-pci.tex
> > > > +++ b/transport-pci.tex
> > > > @@ -375,6 +375,10 @@ \subsubsection{Common configuration structure layout}\label{sec:Virtio Transport
> > > >
> > > >  \item[\field{queue_enable}]
> > > >          The driver uses this to selectively prevent the device from executing requests from this virtqueue.
> > > > +        If the driver negotiates VIRTIO_F_RING_ENABLE_ANYTIME or
> > > > +        VIRTIO_F_RING_RESET, the driver writes 1 to this field to enable a
> > > > +        virtqueue.  See \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Selective Virtqueue Enable}
> > > > +        and \ref{sec:Basic Facilities of a Virtio Device / Virtqueues / Virtqueue Reset}.
> > > >          1 - enabled; 0 - disabled.
> > >
> > > No, this is always used to enable queues even with no features.
> > >
> >
> > Yes. Now I see it is confusing if reading the spec and not the diff,
> > but my intention was to not modify the behavior without the feature
> > flags. Maybe there is a better way to achieve it.
>
> This is bad:
>
>  Driver always does X.
> +If Y then driver does Z.
>
>
> This is good:
>
> -Driver always does X.
> -If Y, then driver does Z, otherwise X.
>
> even though it's a bigger patch, but patch review is
> secondary to a readable spec.
>

I agree, I'll modify it in that direction if we agree on using PCI
config space this way.

Thanks!

>
>
> > > >  \item[\field{queue_notify_off}]
> > > > --
> > > > 2.31.1
> > >
>