RE: [virtio-comment] RE: [PATCH v2 1/2] transport-pci: Introduce legacy registers access commands

[Parav Pandit <parav@nvidia.com>]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, May 24, 2023 4:13 PM
[..]
> > Sounds good to me.
> > What about discovery the notification place for the VF?
> > We have two options.
> > 1. discover them via AQ like config registers access 2. discover it
> > via extended PCI capability
> >
> > #1 is far more programable and does not need to bake in the PCI device
> layout.
> 
> I think 2 is easier for drivers to use.
Yes. 
Its trade off.
But not a huge difference from sw point of view as they use underlying apis.

As opposed to do that baking pci capabilities is slightly hard as it needs to intermix control path over registers..

Typically, CVQ and AQ etc a PCI device would do using some sort of firmware, so adding commands are easier than low level PCI capabilities based tunneling.

> 
> > So this way one day in future we can get rid of it more easily for those PCI
> devices who may prefer everything in the hw.
> 
> I think these devices are better off with a new transport using drastically less
> registers.
> 
That way AQ scores more.

> > > a feels ok to me... maybe test the waters with express capability as
> > > well, it will come handy down the road I think.
> > Yes, if this is really needed for some OS, this addition will be possible in future
> in the spec and also of the devices in CC list here.
> 
> hmm not sure what do you mean. I meant to put the capabilities with the new
> legacy bar in express config space.
> 
I mean to say we should do AQ.
And if an OS can never implement two driver communication for legacy, may be at that future point a spec should evolve to provide this capability-based access.

> > The new capability in mind is:
> > VIRTIO_PCI_EXT_CAP_PCI_LEGACY_REG
> >
> > struct virtio_pci_legacy_cfg_cap {
> > 	struct virtio_pci_cap cap;
> > 	le32 addr; /* register offset, width, op=RD/WR,
> status:0=pending,1=done)
> > 	le32 reg_data;
> > };
> 
> I would use cap64, cap is there because we did not think about 64 bit early
> enough.
>
Yes.
 
> > This will be hard to do for SIOV in future, so I prefer AQ cmd for config
> register access and notification discovery.
> > WDYT?
> 
> I need to go back and look at how does SIOV tvq proposal manage notifications
> then.  Do not remember off the top of my head.
SIOV tvq proposal does not tunnel the driver notifications.
It exposes the address and size of db region like how I had in similar command.
This is why it is not a transport vq and I suggest to rename to aq :)
Snippet of it below.

+The virtqueue notification area information can be get through the following commands:
+
+\begin{lstlisting}
+#define VIRTIO_TRANSPTQ_CTRL_VQ_NOTIFY    11
+ #define VIRTIO_TRANSPTQ_CTRL_VQ_NOTIFY_GET          0
+
+struct virtio_transportq_ctrl_vq_notification_area {
+       u64 address;
+       u64 size;
+};
+\end{lstlisting}