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

Parav Pandit
2025-03-06T03:09:50Z

> From: Sergio Lopez Pascual <slp@redhat.com>
> Sent: Thursday, March 6, 2025 12:39 AM
>
> Parav Pandit <parav@nvidia.com> writes:
>
> >> 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.
>
> I can't think of any more information that would be needed to be added to
> the SHM interface beyond the page size. In a certain sense, it could be argued
> that it should have been there when shared memory regions were first
> introduced, since knowing the alignment restrictions for the maps is a
> requirement to able to make use of the region. It worked so far just because
> 4K page systems/VMs were prevalent.
>
Sounds good. So device sharing the info without the feature bit in this field looks ok to me.

> Thanks!
> Sergio.