Re: [virtio-comment] Re: [PATCH 07/11] transport-pci: Introduce transitional MMR device id

[Parav Pandit <parav@nvidia.com>]

On 4/10/2023 6:18 AM, Michael S. Tsirkin wrote:
> On Sun, Apr 09, 2023 at 03:15:01AM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Friday, April 7, 2023 11:51 AM
> >
> > > > 1. A non-transitional device will expose a capability (not a feature bit, but a
> > > capability at transport level).
> > >
> > > Note that we can allow this capability in transitional devices too.
> > > This is useful since IO bar might not be enabled even if present.
> > This capability exposure makes a device transitional in some sense.
>
> not in the sense spec uses it at the moment: transitional devices
> are those that legacy drivers can bind to. transitional drivers
> btw are those that can bind to legacy devices.
>
> perhaps suprisingly, a transitional driver using a transitional
> device does not rely on any legacy spec at all, they will
> use the standard interfaces.
>
>
> > > > This capability indicates that, it supports legacy interface.
> > > > Lets name it legacy_if_emulation for sake of this discussion.
> > > > It is a two-way pci capability.
> > > > Device reports it.
> > > > And driver enables it. (Why two way and why driver needs to enable it,
> > > described later in point #d below).
> > > > Hence, such non transitional device does not need to comply to below listed
> > > requirements #a and #b.
> > > > a. A driver MUST accept VIRTIO_F_VERSION_1 if it is offered.
> > > > (Because hypervisor driver is a passthrough driver; and legacy driver will not
> > > accept this feature bit).
> > >
> > > This is not a device requirement at all.
> > Those this is written as driver requirement; a device expects this feature bit to be negotiated.
> > What should device implementor do? It should allow driver to not negotiate bit, right?
> >
> > Which means, below line to be change:
> >
> > from:
> > device MAY fail to operate further if VIRTIO_F_VERSION_1 is not accepted.
> >
> > to:
> > Non transitional device that does not have legacy interface capability MAY fail to operate further if V_1 is not accepted.
> > Non transitional device that has legacy interface capability SHOULD operate further even if V_1 is not accepted.
>
>
>
> Look nothing changes with MMR capability at all.
> We currently have:
>
> 	A device MUST offer VIRTIO_F_VERSION_1.  A device MAY fail to operate further
> 	if VIRTIO_F_VERSION_1 is not accepted.
>
> it's implied that this does not refer to legacy interface.
But the interface being exposes is not a legacy interface at the PCI 
device level.

A PCI device is exposing a interface that can be used
by either
a. existing non transitional driver who will negotiate _1, just fine.
or
b. by legacy driver in the guest VM, which will not negotiate _1.

And here device must not fail to operate.
Hence spec should say that it should not fail to operate.

> You want to clarify this adding to legacy interface section text
> explaining that of course VIRTIO_F_VERSION_1 must not
> be offered through that? 
It will be offered because hypervisor driver is not getting involved in 
the reset flow and in read/write of the feature bits etc.

Hypervisor driver is only providing the transport channel from the guest 
vm to the device.

And since the guest driver may be 1.x, _1 will be offered by the device.

> Sure but it's a separate issue
> from MMR capability. don't try to drink the ocean.
>
>
>
>
>
> > > > b. device MAY fail to operate further if VIRTIO_F_VERSION_1 is not accepted.
> > >
> > > This is optional not a requirement.
> > Please see above wording, if its acceptable.
>
>
> you don't need any of that for this effort, generally
> VIRTIO_F_VERSION_1 thing needs a lot of work, if you
> want to invest the time just ask I'll try to list the issues.
>
> But nothing to do with memory mapped legacy interface
> directly.
I likely dont understand your above point.
The point is, a device with new capability needs to

a. offer VERSION_1 offer, allow negotiation VERSION_1
b. allow not negotiation of VERSION_1

With single virtio device id, how to frame this in the spec?
One way I proposed above that a new transport capability indicates this.

I didnt follow your idea of how above #a and #b can be worded without 
the new capability wording.

> > > > c. A non-transitional device with above legacy_if_supported
> > > > capability, will allow device reset sequence, described in [1] Driver
> > > > Requirements: Device Initialization (3.1.1) [2] Legacy Interface:
> > > > Device Initialization (3.1.2)
> > > >
> > > > > > device reset sequence.
> > > > >
> > > > > what is this one?
> > > >
> > > > I listed above in #c.
> > > > And
> > > >
> > > > d. When legacy_if_emulation capability is offered and hypervisor driver
> > > enabled it, when driver perform device reset, driver will not wait for device
> > > reset to go zero.
> > > > When legacy_if_emulation capability is not enabled by (hypervisor or other
> > > say existing) driver, driver will wait for device reset to turn 0. (Following the
> > > driver requirement 2.4.2).
> > >
> > > It might not be a bad idea to enable it, but I observe that it is possible for
> > > hypervisor to expose a standard transitional device on top of this MMR
> > > capability. Thus it will not be known whether guest driver accesses legacy or
> > > modern BAR until guest runs.
> > > I propose, instead, that device exposes same registers at two addresses and
> > > executes reset correctly depending on which address it was accessed through.
> > > WDYT?
> > Yep, this the exact proposal here.
> > Legacy registers exposes via AQ (aka TVQ) or MMR location, behaves like legacy.
> > And regular registers at their location as-is.
> >
> > With that feature bit negotiation is the only thing to relax like worded above.
>
> It's not really different from IO port legacy then.
Yes, it is no different, because what is provided is just the transport, 
not a new functional behavior.