Re: [virtio-dev] [PATCH 08/11] transport-pci: Introduce virtio extended capability

[Jason Wang <jasowang@redhat.com>]

On Tue, Apr 11, 2023 at 3:00âPM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 11, 2023 at 10:19:39AM +0800, Jason Wang wrote:
> > On Mon, Apr 10, 2023 at 6:04âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Apr 10, 2023 at 03:16:46PM +0800, Jason Wang wrote:
> > > > On Mon, Apr 10, 2023 at 2:24âPM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Mon, Apr 10, 2023 at 09:36:17AM +0800, Jason Wang wrote:
> > > > > > On Fri, Mar 31, 2023 at 7:00âAM Parav Pandit <parav@nvidia.com> wrote:
> > > > > > >
> > > > > > > PCI device configuration space for capabilities is limited to only 192
> > > > > > > bytes shared by many PCI capabilities of generic PCI device and virtio
> > > > > > > specific.
> > > > > > >
> > > > > > > Hence, introduce virtio extended capability that uses PCI Express
> > > > > > > extended capability.
> > > > > > > Subsequent patch uses this virtio extended capability.
> > > > > > >
> > > > > > > Co-developed-by: Satananda Burla <sburla@marvell.com>
> > > > > > > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > > > > >
> > > > > > Can you explain the differences compared to what I've used to propose?
> > > > > >
> > > > > > https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg08078.html
> > > > > >
> > > > > > This can save time for everybody.
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > BTW another advantage of extended capabilities is - these are actually
> > > > > cheaper to access from a VM than classic config space.
> > > >
> > > > Config space/BAR is allowed by both of the proposals or anything I missed?
> > > >
> > > > >
> > > > >
> > > > > Several points
> > > > > - I don't like it that yours is 32 bit. We do not need 2 variants just
> > > > >   make it all 64 bit
> > > >
> > > > That's fine.
> > > >
> > > > > - We need to document that if driver does not scan extended capbilities it will not find them.
> > > >
> > > > This is implicit since I remember we don't have such documentation for
> > > > pci capability, anything makes pcie special?
> > >
> > > yes - the fact that there are tons of existing drivers expecting
> > > everything in standard space.
> > >
> > >
> > > > >   And existing drivers do not scan them. So what is safe
> > > > >   to put there? vendor specific? extra access types?
> > > >
> > > > For PASID at least, since it's a PCI-E feature, vendor specific should
> > > > be fine. Not sure about legacy MMIO then.
> > > >
> > > > >   Can we make scanning these mandatory in future drivers? future devices?
> > > > >   I guess we can add a feature bit to flag that.
> > > >
> > > > For PASID, it doesn't need this, otherwise we may duplicate transport
> > > > specific features.
> > >
> > > i don't get it. what does PASID have to do with it?
> >
> > My proposal is to allow PASID capability to be placed on top.
>
> Assuming you mean a patch applied on top of this one.
>
> > So what
> > I meant is:
> >
> > if the driver needs to use PASID, it needs to scan extend capability
> >
> > So it is only used for future drivers. I think this applies to legacy
> > MMIO as well.
>
> sure
>
> > > A new feature will allow clean split at least:
> > > we make any new features and new devices that expect
> > > express capability depend on this new feature bit.
> > >
> > > > >   Is accessing these possible from bios?
> > > >
> > > > Not at least for the two use cases now PASID or legacy MMIO.
> > >
> > > can't parse english here. what does this mean?
> >
> > I meant, it depends on the capability semantics. Both PASID and legacy
> > MMIO don't need to be accessed by BIOS. We can't change legacy BIOS to
> > use legacy MMIO bars.
> >
> > Thanks
>
> makes sense.
>
>
> now, imagine someone building a new device. if existing drivers are not
> a concern, it is possible to move capabilities all to extended space. is
> that possible while keeping the bios working?

This is possible but I'm not sure it's worthwhile. What happens if the
device puts all capabilities in the extended space but the bios can't
scan there? We can place them at both but it then doesn't address the
out of space issue. Things will be easier if we allow new
features/capabilities to be placed on the extended space.

Thanks

>
> > >
> > >
> > > > >
> > > > > So I like this one better as a basis - care reviewing it and adding
> > > > > stuff?
> > > >
> > > > There are very few differences and I will have a look.
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > >
>