[virtio-dev] 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.

---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org