RE: [virtio-comment] RE: [PATCH v3 2/2] content: Support enabling virtqueue after DRIVER_OK stage

[Cornelia Huck <cohuck@redhat.com>]

On Mon, Oct 23 2023, Parav Pandit <parav@nvidia.com> wrote:

> Hi Michael, Cornelia,
>
>> From: Parav Pandit
>> Sent: Thursday, October 19, 2023 7:27 PM
>> 
>> Hi Cornelia, Michael,
>> 
>> > From: virtio-comment@lists.oasis-open.org <virtio-comment@lists.oasis-
>> > open.org> On Behalf Of Parav Pandit
>> > Sent: Wednesday, October 18, 2023 4:43 PM
>> >
>> > > From: Cornelia Huck <cohuck@redhat.com>
>> > > Sent: Wednesday, October 18, 2023 4:34 PM
>> >
>> > > On Wed, Oct 18 2023, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > >
>> > > > On Wed, Oct 18, 2023 at 12:25:23PM +0200, Cornelia Huck wrote:
>> > > >> On Tue, Oct 17 2023, Parav Pandit <parav@nvidia.com> wrote:
>> > > >>
>> > > >> >> From: Cornelia Huck <cohuck@redhat.com>
>> > > >> >> Sent: Tuesday, October 17, 2023 5:55 PM
>> > > >> >>
>> > > >> >> On Mon, Oct 02 2023, Parav Pandit <parav@nvidia.com> wrote:
>> > > >> >> > +When VIRTIO_F_RING_DYNAMIC is not negotiated, the driver
>> > > >> >> > +MUST enable the required number of virtqueues before
>> > > >> >> > +setting the
>> > > DRIVER_OK status bit.
>> > > >> >>
>> > > >> >> What does "required" mean here? It just chooses to enable the
>> > > >> >> queues it wants to use, right?
>> > > >> > Right.
>> > > >> > Required meaning, whatever number of queues that driver choose
>> > > >> > to
>> > > enable, those must be enabled before driver_ok.
>> > > >> > So it is "required by the driver".
>> > > >> > Would that be ok?
>> > > >>
>> > > >> I'd write it as "the driver MUST enable any virtqueue it plans to use"
>> > > >> or something like that.
>> > > >>
>> > > >> (...)
>> > > >
>> > > > It would have to be SHOULD - we can't add new MUST requirements
>> > > > not contingent on a feature bit, we can give recommendation based
>> > > > on existing installed base.
>> > >
>> > It is not just installed base.
>> > Spec clearly says that:
>> > "Perform device-specific setup, including discovery of virtqueues for
>> > the device, optional per-bus setup, reading and possibly writing the
>> > device's virtio configuration space, and population of virtqueues."
>> >
>> > There is no feature bit or any text that says "population of
>> > virtqueues" can be done later outside of the device initialization phase.
>> > Only reference is _F_RESET bit which is only for re-enabling. (not enabling).
>> >
>> > > Then I wonder whether we need to add any normative statement at all
>> > > -- prior to the introduction of this new feature, the driver had to
>> > > enable anything it wanted to use before DRIVER_OK, and if it does
>> > > not negotiate the new feature, nothing changes. Spelling that out in
>> > > non-normative sections should be enough?
>> > It is already implied in the spec that queues must be enabled before
>> > DRIVER_OK.
>> > So what is written in normative is not changing any behavior.
>> > It is only making the implied behavior explicit.
>> >
>> > If it says SHOULD, that means, one can enable the queue for the first
>> > time after DRIVER_OK stage too.
>> > And that would violate the "Driver Requirements: Device Initialization".
>> >
>> 
>> I have addressed the comments of v3 in v4 at [1].
>> The device requirement is kept as MUST to keep it aligned with the "driver
>> requirements section".'
>> 
>> [1] https://lore.kernel.org/virtio-comment/20231017143135.758523-1-
>> parav@nvidia.com/T/#mc49b6e54186c5f1b1e8c743bc440ca0cc0938212
>> 
>> Please review.
>
> If no comments in v4 which was a week ago, please raise the voting ballot.

Well, there are  still comments from me here that post-date v4 (please
wait for a bit before posting another version!), so I'll continue
waiting for them to be addressed first.

>
> [1] https://lists.oasis-open.org/archives/virtio-comment/202310/msg00183.html
>
> Parav