RE: [PATCH 1/3] shared-mem: introduce page alignment restrictions

Parav Pandit
2025-03-05T11:19:01Z

> From: Sergio Lopez Pascual <slp@redhat.com>
> Sent: Wednesday, February 26, 2025 11:20 PM
> Parav Pandit <parav@nvidia.com> writes:
>
> >> From: Sergio Lopez <slp@redhat.com>
> >> Sent: Monday, February 17, 2025 5:22 PM
> >>
> >> Add a subsection for page alignment restrictions and introduce the
> >> VIRTIO_F_SHM_PAGE_SIZE feature bit.
> >>
> >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> >> ---
> >> content.tex | 3 +++
> >> shared-mem.tex | 7 +++++++
> >> 2 files changed, 10 insertions(+)
> >>
> >> diff --git a/content.tex b/content.tex index c17ffa6..575065d 100644
> >> --- a/content.tex
> >> +++ b/content.tex
> >> @@ -872,6 +872,9 @@ \chapter{Reserved Feature
> >> Bits}\label{sec:Reserved Feature Bits}
> >> \ref{devicenormative:Basic Facilities of a Virtio Device / Feature
> >> Bits} for
> >> handling features reserved for future use.
> >>
> >> + \item[VIRTIO_F_SHM_PAGE_SIZE(42)] This feature indicates that the
> >> + device transport provides information about the supported page size.
> >> +
> >
> > Please extend the description around "Feature Bits" section to increase 24
> to 41 -> 24 to 42.
>
> Ack.
>
> >> \end{description}
> >>
> >> \drivernormative{\section}{Reserved Feature Bits}{Reserved Feature
> >> Bits} diff --git a/shared-mem.tex b/shared-mem.tex index
> >> 6e6f6c4..dd90cb7 100644
> >> --- a/shared-mem.tex
> >> +++ b/shared-mem.tex
> >> @@ -34,6 +34,13 @@ \subsection{Addressing within
> >> regions}\label{sec:Basic Facilities of a Virtio De The \field{shmid}
> >> may be explicit or may be inferred from the context of the reference.
> >>
> >> +\subsection{Page alignment restrictions}\label{sec:Basic Facilities
> >> +of a Virtio Device / Shared Memory Regions / Page alignment
> >> +restrictions}
> >> +
> >> +If VIRTIO_F_SHM_PAGE_SIZE has been negotiated, when requesting the
> >> +device to map memory into a shared memory region, the driver MUST
> >> +obtain the page size information from the transport and honor the
> >> +page alignment constrains derived from that page size.
> >> +
> > A device requirement is also needed to indicate that pci capability field is
> only valid after/when the feature is negotiated.
>
> Ack.
I don’t think this is necessary because you say "driver obtains the information".
So to obtain a free information, feature bit is not necessary, this can be self-described in the pci capability itself.
If a pci capability padding_bit_0 is set, it can tell rest of the bits have abc meaning.
This way pci capabilities are self-contained.

>
> >> \devicenormative{\subsection}{Shared Memory Regions}{Basic
> >> Facilities of a Virtio Device / Shared Memory Regions} Shared
> >> memory regions MUST NOT expose shared memory regions which
> >> --
> >> 2.48.1
> >>
> > 1. Shared memory cap is already defined as
> VIRTIO_PCI_CAP_SHARED_MEMORY_CFG.
> > This capability should be extended instead of polluting generic structure
> field of padding.
> >
> > If its done in generic way like proposed, you need to call out that page_size
> field is only valid for VIRTIO_PCI_CAP_SHARED_MEMORY_CFG.
> >
> > Overall extending pci cap structure is not good idea even though it may
> appear as small change for below reasons.
> >
> > 1. PCI spec already ran out of total bytes that can be stored in the capability
> section. Extending it will not help.
> > A virtio level extended capability is not good either due to below guidance
> from PCI-SIG.
> >
> > 2. PCI spec highly discouraged putting vendor specific bits like this in the
> capability section.
> > Citation: "It is strongly recommended that PCI Express devices place
> > no registers in Configuration Space other than those in headers or Capability
> structures architected by applicable PCI specifications."
>
> Honestly, making it clear that the field is only valid for
> VIRTIO_PCI_CAP_SHARED_MEMORY_CFG _and_ when
> VIRTIO_F_SHM_PAGE_SIZE has been negotiated (I can extend the comment of
> the field itself to clarify that, otherwise, it just acts as padding), I fail to see
> which kind of problems might bring, but admitedly I'm not a PCI expert by any
> means.
>
Feature negotiation for information exposed by the device is not needed.

> If using the padding is really not an option, would it be reasonable to
> introduce a different variant of "virtio_pci_cap64" struct, including the
> page_size, to be used when VIRTIO_F_SHM_PAGE_SIZE has been negotiated?
>
It can be extended. It just not right way to do so in pci specs boundary.

> IIUC, "virtio_pci_cap64" is VIRTIO-specific, so this shouldn't have impact
> anywhere else.
>
> > So, you should consider discovering the shared memory page size and more
> such fields using device administration commands.
> >
> > It may sound overkill to use administration commands for 8-bits of
> > page size, but as more generic and device specific things evolve, it is likely
> useful vehicle where you don't have to search hard to squeeze things in some
> existing structure like pci capability.
>
> Do you mean using an adminitration queue (VIRTIO_F_ADMIN_VQ)? If so, yes,
> I think it's extremely overkill for both the device and the driver.
> If possible, I would like explore every other option before resorting to this
> one.
>
Right. I agree that exposing this information via admin command is overkill.
Do you agree that shared memory interface wont grow anymore further, only thing it needs now or in any near term is just one or two bytes of page size information?
If yes, packing inside the pci cap seems fine.
If one needs to start expanding this more, than more mature interface like admin cmd will be useful.