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

[Jason Wang <jasowang@redhat.com>]

On Tue, Apr 11, 2023 at 1:54âAM Parav Pandit <parav@nvidia.com> wrote:
>
> Hi Jason,
>
> On 4/9/2023 9:36 PM, 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.
> >
>
> What is proposed in this patch similar to [1].
>
> The main difference is, the proposed new capability is always placed in
> the pci extended capability section,
> This is because legacy capability section is nearly to its full level as
> described in commit message.
>
> So providing it at either of the two locations is not valuable.
>
> What you proposed in [1] is in general useful regardless;
>
> However, it is not backward compatible, if the device place them in
> extended capability, it will not work.
>

It is kind of intended since it is only used for new PCI-E features:

"
+The location of the virtio structures that depend on the PCI Express
+capability are specified using a vendor-specific extended capabilities
+on the extended capabilities list in PCI Express extended
+configuration space of the device.
"

> To make it backward compatible, a device needs to expose existing
> structure in legacy area. And extended structure for same capability in
> extended pci capability region.
>
> In other words, it will have to be a both places.

Then we will run out of config space again? Otherwise we need to deal
with the case when existing structures were only placed at extended
capability. Michael suggest to add a new feature, but the driver may
not negotiate the feature which requires more thought.

>
> Otherwise its similar.
>
> We should do this regardless, and it will also make this series shorter
> which is also what Michael prefers.
>
> Would you like join efforts with me of drafting [1] + above description
> as independent patch?

I think it should be fine but I'm not sure what it looks like.

> We may need it even sooner than this because the AQ patch is expanding
> the structure located in legacy area.

Just to make sure I understand this, assuming we have adminq, any
reason a dedicated pcie ext cap is required?

Thanks

>
> [1]
> https://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg08078.html
>
> PASID part of the patch is not relevant here, so will skip to comment.
>