Re: [virtio-comment] RE: [PATCH v5 2/7] Introduce admin command set

[Jason Wang <jasowang@redhat.com>]

On Wed, Jun 29, 2022 at 5:02 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jun 29, 2022 at 04:43:18PM +0800, Jason Wang wrote:
> > On Tue, Jun 28, 2022 at 10:24 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Thu, Jun 23, 2022 at 11:34:42AM +0800, Jason Wang wrote:
> > > > On Thu, Jun 23, 2022 at 10:57 AM Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > >
> > > > > > From: Jason Wang <jasowang@redhat.com>
> > > > > > Sent: Wednesday, June 22, 2022 10:42 PM
> > > > >
> > > > > [..]
> > > > > > > Are you suggesting only feature bits through register-based transport or all
> > > > > > commands via register-based transport?
> > > > > >
> > > > > > For feature bits, if we don't have admin virtqueue as transport,
> > > > > Right.
> > > > > Management tasks (features) done via AQ are negotiated via the AQ (regardless of device type being management dev or managed dev).
> > > > > It is not replacing usual feature bits negotiation that has strong relationships with device initialization sequence.
> > > >
> > > > Just to clarify,
> > > >
> > > > E.g if the admin virtqueue is implemented in PF. I suggest using the
> > > > existing PCI transport based feature negotiation since:
> > > >
> > > > 1) we have selector, so we don't need any new registered and we don't
> > > > need worry about the scalability
> > >
> > >
> > > I think this is a point that needs addressing.
> > >
> > > To be more specific. The concern raised was that feature bits are memory
> > > mapped. Accessing them on PCI requires an MMIO write. According to the
> > > PCI spec it is not legal for a device to defer accepting a write until
> > > another write is accepted (since that leads to deadlocks).
> > > Thus any info programmed using MMIO writes has to reside in
> > > on-card memory and can not be offloaded to system RAM.
> >
> > Probably, but it can be offloaded via other ways. We had already used
> > 2 for e.g feature_sel, it works for the past 5+ years, I guess having
> > 4 more should work for the future 10 years. We only have this "issue"
> > for some specific hardware (e.g we used to have similar discussion
> > when somebody posted pci endpoint device support for virtio-net).
> >
> > Note that this is only an "issue" for the management device (PF),
>
> The claim was that for nesting purposes we might want to add
> admin queue to the VFs as well.

So at least from the current usage (MSI-X allocation), I don't see why
an admin queue is needed for VF.

And in the case of nesting, it would be much easier to use MMIO
registers. Qemu is free to map it to any function of the device.

>
> > having several more bytes of register just for the PF doesn't seem
> > expensive. We know we can use admin virtqueue as a transport for
> > managed devices.
>
> Did you post a prototype spec patch of this at some point? I do not recall.

I did it here.

https://lists.oasis-open.org/archives/virtio-comment/202108/msg00025.html

Thanks

>
> > >
> > > After thinking about these issues I have an idea:
> > >
> > > At the moment VQs are already never programmed before FEATURES_OK,
> > > features are never programmed after FEATURES_OK.
> > > This implies that device can actually store features in
> > > VQ state registers temporarily until FEATURES_OK.
> > > This is more than 24 bytes of memory per VQ, with at least
> > > one data VQ and one admin VQ, this will be sufficient for a long time.
> >
> > Something like
> >
> > struct virtio_pci_common_cfg {
> >       union {
> >             virtqueue_state;
> >              features;
> >        }
> > }
> >
> > ?
>
>
> Yea! Except in verilog.
>
> > >
> > > Thus, all that we need to do is prohibit programming VQs
> > > before FEATURES_OK more strongly.
> > > I can work on that if the idea is acceptable to others.
> >
> > Note sure, it looks to me if such kind of device is popular, it might
> > be better to have new PCI transport for non-register behaviour
> > devices.
> >
> > Thanks
> >
> > >
> > >
> > >
> >
> > >
> > > --
> > > MST
> > >
>