RE: [virtio-comment] [PATCH 5/5] virtio-pci: implement VIRTIO_F_QUEUE_STATE

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, September 19, 2023 9:56 AM

> On Sun, Sep 17, 2023 at 1:29âPM Parav Pandit <parav@nvidia.com> wrote:
> >
> >
> > > From: Jason Wang <jasowang@redhat.com>
> > > Sent: Thursday, September 14, 2023 8:42 AM
> > >
> > > On Wed, Sep 13, 2023 at 12:46âPM Parav Pandit <parav@nvidia.com>
> wrote:
> > > >
> > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > Sent: Wednesday, September 13, 2023 10:14 AM
> > > >
> > > > > It's not about how many states in a single state machine, it's
> > > > > about how many state machines that exist for device status.
> > > > > Having more than one creates big obstacles and complexity in the
> > > > > device. You need to define the interaction of each state
> > > > > otherwise you leave undefined
> > > behaviours.
> > > > The device mode has zero relation to the device status.
> > >
> > > You will soon get this issue when you want to do nesting.
> > >
> > I donât think so. One needs to intercept it when one wants to do
> trap+emulation which seems to fullfil the nesting use case.
> 
> Well, how can you trap it? You have admin vq in L0, it means the suspending is
> never exposed to L1 unless you assign the owner to L1.
> Is this what you want?
> 
When nesting is not done, it is not needed.
Only the nest cases need to trap it.
So when one want to do nesting use case, one should also place the admin peer PF in that guest.
Right assign one VF and its peer admin VF to L1.

> >
> > > > It does not mess with it at all.
> > > > In fact the new bits in device status is making it more complex
> > > > for the device
> > > to handle.
> > >
> > > Are you challenging the design of the device status? It's definitely
> > > too late to do this.
> > >
> > No. I am saying the extending device_status with yet another state is equally
> complex and its core of the device.
> 
> You never explain why.
If you are comparing two methods, then a new feature adds complexity.
Hence, they both score equal adding complexity for new feature.
In case of device_status one needs to things synchronously.
This adds complexity on the device side to answer those registers in hot downtime path.
When done over admin commands, they happen in parallel.
 
> 
> >
> > > This proposal increases just one bit and that worries you? Or you
> > > think one more state is much more complicated than a new state
> > > machine with two states?
> >
> > It is mode and not state. And two modes are needed for supporting P2P
> device.
> 
> You keep saying you are migrating the core virtio devices but then you are
> saying it is required for PCI. And you never explain why it can't be done by
> reusing the device status bit.
It cannot be done using device status bits, because hypervisor is not involved in trapping, and parsing it.
We better discuss in the actual series where things are posted.

> 
> > When one wants to do with mediation, there also two states are needed.
> >
> > The key is modes are not interacting
> 
> You need to explain why they are not interacting. It touches the virtio facility
> which (partially) overlaps the function of the device status for sure. You invent a
> new state machine, and leave the vendors to guess how or why they are not
> interacting with the existing one.
Huh, something needs explanation when there is interaction.
I explained that device_status is just another virtio register.

I missed to add the other vendors Sign-off. I will add it.

> There are just too many corner cases that need to be figured out.
> 
> For example:
> 
> How do you define stop? Is it a virtio level stop, transport level or a mixing of
> them both? 
It is defined in the series and device and driver requirements section and also in theory of operation.

> Is the device allowed to stop in the middle or reset, feature
> negotiation or even transport specific things like FLR?
Yes.

> If yes, how about other operations and who defines and maintains those
> transitional states? If not, why and how long would a stop wait for an operation?
> Can a stop fail? What happens if the driver wants to reset but the device is
> stopped by the admin commands? Who suppresses who and why?
All are described in the series in the normative. If something is missing, please put comment there and I will fix in v1.

> 
> This demonstrates the complexity of your proposal and I don't see any of the
> above were clearly stated in your series. Reusing the existing device status
> machine, everything would be simplified.
You see, too early conclusion saying things are missing, take mine...
I will fix missing items in v1, please put review comments in there.

> 
> > with the device_status because device_status is just another register of the
> virtio.
> 
> Let's don't do layer violation, device status is the basic facility of the virtio
> device which is not coupled with any transport so it is not necessarily
> implemented via registers.

There is no violation. Device_status is already part of the transport specific context.