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

[Jason Wang <jasowang@redhat.com>]

On Fri, May 19, 2023 at 3:42âAM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, May 17, 2023 at 07:32:33PM +0000, Parav Pandit wrote:
> > > From: Michael S. Tsirkin <mst@redhat.com>
> > > Sent: Wednesday, May 17, 2023 1:45 AM
> > >
> > > On Sat, May 06, 2023 at 03:01:34AM +0300, Parav Pandit wrote:
> > > > +\begin{lstlisting}
> > > > +struct virtio_admin_cmd_lreg_wr_data {
> > > > + u8 offset; /* Starting byte offset of the register(s) to write */
> > > > + u8 register[];
> > > > +};
> > > > +\end{lstlisting}
> > >
> > > So to summarize, I think the main comment here was to have separate
> > > commands for access to common versus device config. Just today I thought
> > > some more and came up with another reason why this is a good idea: if we ever
> > > want legacy MMIO emulation, we can reuse the device config command.
> > >
> > MMIO is actually simpler than PCI because all the registers are contiguous regardless of msix.
> > So single command with offset can service just fine.
>
> yes but then you need to implement both MMIO and PCI in hardware.
> with a flag you can implement legacy MMIO in software on top of
> legacy PCI in hardware.
>
> > More below.
> >
> > > Jason also feels the common config command can be shared with vq transport
> > > effort. We can discuss this, for sure, I guess its putting code in hardware versus
> > > in hypervisor, but if there are hardware vendors who strictly want this code in
> > > hardware I don't have a big problem with this even if I don't exactly get why.
> > >
> > > With this idea all hypervisor has to do is subtract the offset from device config
> > > access, sounds like a small price to pay.
> > It is more than the subtraction cost.
> >
> > Hypervisor needs to inspect MSI-X enable/disable settings to decide when to issue command #1 vs #3 for the requested offset.
>

I don't know why it is a problem, for any hypervisor that supports
legacy, the code (MSI-X enablement detection) should already be there.

And it is just something like what Qemu had:

/* The remaining space is defined by each driver as the per-driver
 * configuration space */
#define VIRTIO_PCI_CONFIG_SIZE(dev)     VIRTIO_PCI_CONFIG_OFF(msix_enabled(dev))

Is this really expensive?

> I thought that for now you wanted to ignore MSI-X disable case?
> Because if you do care then we need INTx emulation.
>
>
> > > Does this sound like a reasonable
> > > compromize to you?
> >
> > Splitting proposed one command to two commands,
> > 1. one for accessing legacy common config
> > 2. second for accessing legacy device specific config
> >
> > seems fine to me as below.
> >
> > So we will have total 5 commands (instead of 3).
> >
> > 1. legacy common config read
> > 2. legacy common config write
> >
> > 3. legacy device config read
> > 4. legacy device config write
> > 5. query device notification area
> >
> > #1 and #3 same cmd signature but different opcode.
> > #2 and #4 same cmd signature but different opcode.
> >
>
> Sounds reasonable. Jason?

Better, but for transport virtqueue/adminq we won't go this way but
using individual commands for common and other configurations for a
more deterministic behaviour with a simpler device.

>
> notification thing needs more thought I feel though.
> It feels weirdly bolted on, but I can't put my finger
> on what's wrong exactly yet. Will think it over.

Two things in my mind:

1) There's probably no need for a BAR and it should be sufficient for
an address, the device just needs to make sure to report a correct
address if BAR is remapped. It helps to save BAR for VF as well.
2) We need a size, otherwise we don't know if the notification area
could be mapped safely to the guest. And in the case of
virtio_pci_notify_cap, though it could be implied by the offset but it
deserves a formal clarification as well there.

Thanks

>
>
> >
> > This publicly archived list offers a means to provide input to the
> > OASIS Virtual I/O Device (VIRTIO) TC.
> >
> > In order to verify user consent to the Feedback License terms and
> > to minimize spam in the list archive, subscription is required
> > before posting.
> >
> > Subscribe: virtio-comment-subscribe@lists.oasis-open.org
> > Unsubscribe: virtio-comment-unsubscribe@lists.oasis-open.org
> > List help: virtio-comment-help@lists.oasis-open.org
> > List archive: https://lists.oasis-open.org/archives/virtio-comment/
> > Feedback License: https://www.oasis-open.org/who/ipr/feedback_license.pdf
> > List Guidelines: https://www.oasis-open.org/policies-guidelines/mailing-lists
> > Committee: https://www.oasis-open.org/committees/virtio/
> > Join OASIS: https://www.oasis-open.org/join/
>