Re: [PATCH v10 4/4] transport-pci: Introduce group legacy group member config region access

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jul 06, 2023 at 05:47:08PM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Thursday, July 6, 2023 1:34 PM
> > 
> > On Thu, Jul 06, 2023 at 04:58:51PM +0000, Parav Pandit wrote:
> > >
> > >
> > > > From: Michael S. Tsirkin <mst@redhat.com>
> > > > Sent: Thursday, July 6, 2023 12:42 PM
> > > >
> > > > We will need to detail how notifications work though. The reason is
> > > > that the legacy notifications are received by owner driver (which is
> > > > not legacy!) and forwarded to device.
> > > > We need to explain exactly how this is done for people who don't
> > > > read minds and did not follow 3 months of mailing list debates, and
> > > > explain how this has the same effect as triggering driver notification through
> > the legacy interface.
> > > >
> > > I am not going to write driver implementation in the spec here.
> > > But I agree some high-level text is needed.
> > > With your suggestions
> > > 1. to merge pci and generic in one section, 2. add description to
> > > notification, 3. past rewrites provided in v9,
> > >
> > > The draft of v11, looks like below. Please see how much more verbose to make
> > it, if at all.
> > >
> > > \paragraph{Legacy Driver Notification}\label{par:Basic Facilities of a
> > > Virtio Device / Device groups / Group administration commands / Legacy
> > > Interface / Legacy Driver Notifications}
> > >
> > > If the group owner device or the group member device support driver
> > > notifications via a memory-mapped operation or I/O operation, these
> > > notifications are sent to the device via accessing such a notification
> > > region using a memory or I/O operation instead of sending the
> > > notifications through the administration command.
> > 
> > This paragraph adds very little information - just some vague hints at how things
> > work.
> > 
> A hunk added this paragraph back.
> Going to remove it as description is duplicate of what is written below.
>
> > > The driver of the owner device can send a driver notification to the
> > > member device by executing
> > VIRTIO_ADMIN_CMD_LEGACY_COMMON_CFG_WRITE
> > > with the \field{offset} matching \field{Queue Notify} and the
> > > \field{data} containing the virtqueue index to be notified.
> > >
> > > However, as many administration commands are used for slow path
> > > configuration, a separate fast path mechanism for such notifications
> > > is desired. For the SR-IOV group type, the optional command
> > > VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO
> > > addresses this need by returning to the driver one or more addresses
> > > which to be used to send such driver notifications.
> > >
> > > In one example, the driver in the hypervisor which intercepts I/O BAR
> > > accesses for the \field{Queue Notify} can access the notification area
> > > directly as memory or I/O access (depending on what the device
> > > reported) instead of slow administration command.
> > >
> Above paragraph is really more than enough for the reader to implement.
> I will merge your text of 16-bit notify vq line here.

I don't have all of the context in front of me. We'll see in v11.
Try to keep a style consistent with what we have written
for the config space.


> > > For the command VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO the driver
> > sets
> > > \field{opcode} to 0x6.
> > > The driver sets \field{group_member_id} which refers to the member
> > > device to be accessed. This command does not use
> > \field{command_specific_data}.
> > >
> > > This command is currently only defined for the SR-IOV group type. When
> > > the device supports VIRTIO_ADMIN_CMD_LEGACY_NOTIFY_INFO command,
> > the
> > > group owner device hardwires VF BAR0 to zero in the SR-IOV Extended
> > > capability and the group member device does not use PCI BAR0 in all
> > > the Virtio PCI capabilities listed in section \ref{sec:Virtio Transport Options /
> > Virtio Over PCI Bus / Virtio Structure PCI Capabilities}.
> > 
> > After reading this I still have no clue at how these notifications work.
> > maybe it's in some other patch. Let's see v11.
> >
> There is only one patch now in v11 other than two fixes.
> 
> > Just making a wild guess:
> > 
> > it is true that
> > 	to supply a driver notification to the device, it is possible for the driver
> > to write the vq index of the vq to be notified, in little endian format, using a
> > single 16 byte write at the address provided ?
> > 
> > and I am guessing that
> > 	the write is an IO or a memory write, depending on the
> > 	type of the BAR referred to?
> > 
> > then why don't we just say this?
> All this while I assume that what you wrote above is already written in description for "queue notify" register in " Legacy Interfaces: A Note on PCI Device Layout".
> So no need to repeat.
> 
> But I realized that base spec is totally silent on it, but still we are supposed to write here.
> Ok.
> So yes, let me add above line here, without touching existing legacy interface section.

It's not documented all that well. I think it was clearer originally
but with all the VIRTIO_F_NOTIFICATION_DATA and such it bit-rotted.

But again even if you add it there, you can not claim it's exactly the
same as legacy because the address is different, the address type is
different, the driver is different (this is owner driver) and even
the device is different.

Please also call out the unusual configuration where the type
is "member" and then you have the owner driver access the
memory of the member device. People might be confused.

I also think we should explain that order of entries is a hint
to driver: use the 1st entry that you can.

-- 
MST