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

["Michael S. Tsirkin" <mst@redhat.com>]

On Sun, Feb 23, 2025 at 05:21:00AM +0000, Parav Pandit wrote:
> 
> > 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.
> 
> >  \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.
> 
> >  \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."
> 
> 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.

Or just in config space, really.


> I skipped reviewing patch 2 and 3, and would like to understand your thoughts for above considerations.

There's actually a demand from embedded folks to allow the capabilities
in a fixed offset in a BAR instead of in config space.