Re: [PATCH V2 1/2] virtio-pci: introduce virtio structure PCI Extended Capability

[Stefan Hajnoczi <stefanha@redhat.com>]

On Wed, Jan 12, 2022 at 01:57:54PM +0800, Jason Wang wrote:
> We're already out of the configuration space if there's a device that
> supports all kinds of the virtio structure via PCI capability. This
> prevents us from adding new capabilities in the future.
> 
> So the patch adds the support for virtio structure via PCI Extended
> Capability via the vendor specific extended capability.
> 
> Only MMIO bar is allowed now.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  content.tex | 127 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
> 
> diff --git a/content.tex b/content.tex
> index cf20570..00a75f2 100644
> --- a/content.tex
> +++ b/content.tex
> @@ -1476,6 +1476,129 @@ \subsubsection{Non-transitional Device With Legacy Driver: A Note
>     of BAR0 by presenting zeroes on every BAR and ignoring writes.
>  \end{enumerate}
>  
> +\subsection{Virtio Structure PCI Extended Capabilities}\label{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Extended Capabilities}
> +
> +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.

I can't parse this sentence. Can you rephrase it and/or split it into
multiple sentences?

> The virtio structure extended
> +capability uses little-endian format: all fields are read-only for the
> +driver unless stated otherwise:
> +
> +\begin{lstlisting}
> +struct virtio_pci_ecap {
> +        le16 cap_vndr;      /* Generic PCI field: PCI_EXT_CAP_ID_VNDR */
> +        le16 cap_rev:4;     /* Generic PCI field: capability version: 0x1 */
> +        le16 cap_next:12;   /* Generic PCI field: next ptr */
> +        le16 cfg_type;      /* Identifies the structure */
> +        le16 cfg_rev:4;     /* Identifies the version of the structure: 0x1 */
> +        le16 cap_len:12;    /* The bytes of the entire capability */
> +        u8 bar;             /* Where to find it. */
> +        u8 id;              /* Multiple capabilities of the same type */
> +        u8 padding[2];      /* Pad to full dword. */
> +        le32 offset;        /* Offset within bar. */
> +        le32 length;        /* Length of the structure, in bytes. */
> +};
> +\end{lstlisting}
> +
> +This structure can be followed by extra data, depending on the
> +\field{cfg_type}.
> +
> +\begin{description}
> +\item[\field{cap_vndr}]
> +        0x0B; Identifies a vendor-specific extended capability.

It's worth clarifying that "vendor-specific" here refers to the PCIe
standard, not to the VIRTIO standard. Please use "PCI Vendor-Specific
Extended Capability" everywhere in this patch instead of just
"vendor-specific extended capability". That helps avoid confusion with
VIRTIO_PCI_CAP_VENDOR_CFG.

> +
> +\item[\field{cap_rev}]
> +        0x01; Identifies the version of the capability structure.
> +
> +\item[\field{cap_next}]
> +        Link to next extended capability in the capability list in the
> +        PCI Express extended configuration space.
> +
> +\item[\field{cfg_type}]
> +        Identifies the structure. All values are reserved for future
> +        use.
> +
> +        The device MAY offer more than one structure of any type - this makes it
> +        possible for the device to expose multiple interfaces to drivers.  The order of
> +        the capabilities in the capability list specifies the order of preference
> +        suggested by the device. A device MAY specify that this ordering mechanism be
> +        overridden by the use of the \field{id} field.
> +
> +\item[\field{cfg_rev}]
> +        0x01; Identifies the version of virtio structure PCI extended
> +        capability.
> +
> +\item[\field{cap_len}]
> +        The length of the entire vendor specific extended capability,
> +        including the virtio_pci_ecap structure and vendor specific
> +        registers.

I thought the point of this is to place the register in ->bar, so why
would there be extra registers after struct virtio_pci_ecap(64)? Again,
it's unclear who the "vendor" is.

> @@ -1488,6 +1611,10 @@ \subsubsection{Device Initialization}\label{sec:Virtio Transport Options / Virti
>  PCI capability list, detecting virtio configuration layout using Virtio
>  Structure PCI capabilities as detailed in \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Capabilities}
>  
> +Optionally, if the device is a PCIe device, the driver scans the PCI
> +Extended capability list, detecting virtio configuration layout using
> +Virtio Struct PCI Extended capabilities as detailed in \ref{sec:Virtio Transport Options / Virtio Over PCI Bus / Virtio Structure PCI Extended Capabilities}

If the same structure is present in both the PCI Capabilities and PCI
Extended Capabilities lists, which one has a higher priority?

Attachment: signature.asc
Description: PGP signature