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

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

[Sergio Lopez]

There's an incresing number of machines supporting multiple page sizes
and, on these machines, the host and a guest can be running with
different pages sizes.

In addition to this, there might be devices that have a required and/or
preferred page size for mapping memory.

In this series we extend the "Shared Memory Regions" with a subsection
explaining the posible existence of page alignment restrictions.

For the device to provide the page size information to the driver, we
need to extend the PCI and MMIO transports. For the former, we borrow
8 bits from the 16 bit padding in virtio_pci_cap to hold a page_shift
field which can be used to derive the page size by using the following
formula: (page_size = 1 << (page_shift + 12)).

For MMIO, we add a the SHMPageShift register at offset 0x0c4, also
holding the page_shift value. Since MMIO registers are 32 bit wide, we
could have asked the device to directly provide page_size instead of
page_shift, but seems reasonable to be consistent across transports.

Since accessing a new MMIO register could be interpreted by the device
as an error, this behavior is gated behind the newly introduced
VIRTIO_F_SHM_PAGE_SIZE feature bit.

An implementation of the changes proposed in this series has been
published as an RFC to the LKML, to be used as a reference:

https://lore.kernel.org/all/20250214-virtio-shm-page-size-v2-0-aa1619e6908b@redhat.com/

Sergio Lopez (3):
  shared-mem: introduce page alignment restrictions
  transport-pci: VIRTIO_F_SHM_PAGE_SIZE support
  transport-mmio: VIRTIO_F_SHM_PAGE_SIZE

 content.tex        |  3 +++
 shared-mem.tex     |  7 +++++++
 transport-mmio.tex |  8 ++++++++
 transport-pci.tex  | 10 +++++++++-
 4 files changed, 27 insertions(+), 1 deletion(-)

-- 
2.48.1

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

[Sergio Lopez]

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.
+
 \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.
+
 \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

[PATCH 2/3] transport-pci: VIRTIO_F_SHM_PAGE_SIZE support

[Sergio Lopez]

Introduce VIRTIO_F_SHM_PAGE_SIZE support by borrowing 8 bits from the 16
bit padding in virtio_pci_cap to hold a page_shift field which can be
used to derive the page size supported by the device.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 transport-pci.tex | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/transport-pci.tex b/transport-pci.tex
index a5c6719..1b91d8a 100644
--- a/transport-pci.tex
+++ b/transport-pci.tex
@@ -147,7 +147,8 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
         u8 cfg_type;    /* Identifies the structure. */
         u8 bar;         /* Where to find it. */
         u8 id;          /* Multiple capabilities of the same type */
-        u8 padding[2];  /* Pad to full dword. */
+        u8 page_shift;  /* Page shift for this shared memory region. */
+        u8 padding[1];  /* Pad to full dword. */
         le32 offset;    /* Offset within bar. */
         le32 length;    /* Length of the structure, in bytes. */
 };
@@ -222,6 +223,13 @@ \subsection{Virtio Structure PCI Capabilities}\label{sec:Virtio Transport Option
         of a certain type. If the device type does not specify the meaning of
         this field, its contents are undefined.
 
+\item[\field{page_shift}]
+        If VIRTIO_F_SHM_PAGE_SIZE has been negotiated and \field{cfg_type} is set
+        to VIRTIO_PCI_CAP_SHARED_MEMORY_CFG, the device MUST provide the page
+        shift derived from the supported page size for this shared memory region.
+        The page shift is derived from the page size by using this formula:
+        $page size = 1 << (page shift + 12)$.
+        See also \ref {sec:Basic Facilities of a Virtio Device / Shared Memory Regions / Page alignment restrictions}.
 
 \item[\field{offset}]
         indicates where the structure begins relative to the base address associated
-- 
2.48.1

[PATCH 3/3] transport-mmio: VIRTIO_F_SHM_PAGE_SIZE

[Sergio Lopez]

Introduce VIRTIO_F_SHM_PAGE_SIZE by adding a the new register
"SHMPageShift" to hold the value that can be used to derive the page
size value supported by the device.

Signed-off-by: Sergio Lopez <slp@redhat.com>
---
 transport-mmio.tex | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/transport-mmio.tex b/transport-mmio.tex
index 94a93a1..28d1594 100644
--- a/transport-mmio.tex
+++ b/transport-mmio.tex
@@ -242,6 +242,14 @@ \subsection{MMIO Device Register Layout}\label{sec:Virtio Transport Options / Vi
     apply to the queue selected by writing to \field{QueueSel}.
   }
   \hline
+  \mmioreg{SHMPageShift}{Shared memory region page shift 8 bit long}{0x0c4}{R}{%
+    If VIRTIO_F_SHM_PAGE_SIZE has been negotiated, this register returns
+    the page shift derived from the supported page size for this shared
+    memory region. The page shift is derived from the page size by using
+    this formula: $page size = 1 << (page shift + 12)$.
+    See also \ref {sec:Basic Facilities of a Virtio Device / Shared Memory Regions / Page alignment restrictions}.
+  }
+  \hline
   \mmioreg{ConfigGeneration}{Configuration atomicity value}{0x0fc}{R}{
     Reading from this register returns a value describing a version of the device-specific configuration space (see \field{Config}).
     The driver can then access the configuration space and, when finished, read \field{ConfigGeneration} again.
-- 
2.48.1

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

[Dmitry Osipenko]

On 2/17/25 14:52, Sergio Lopez wrote:
> There's an incresing number of machines supporting multiple page sizes
> and, on these machines, the host and a guest can be running with
> different pages sizes.
> 
> In addition to this, there might be devices that have a required and/or
> preferred page size for mapping memory.
> 
> In this series we extend the "Shared Memory Regions" with a subsection
> explaining the posible existence of page alignment restrictions.
> 
> For the device to provide the page size information to the driver, we
> need to extend the PCI and MMIO transports. For the former, we borrow
> 8 bits from the 16 bit padding in virtio_pci_cap to hold a page_shift
> field which can be used to derive the page size by using the following
> formula: (page_size = 1 << (page_shift + 12)).
> 
> For MMIO, we add a the SHMPageShift register at offset 0x0c4, also
> holding the page_shift value. Since MMIO registers are 32 bit wide, we
> could have asked the device to directly provide page_size instead of
> page_shift, but seems reasonable to be consistent across transports.
> 
> Since accessing a new MMIO register could be interpreted by the device
> as an error, this behavior is gated behind the newly introduced
> VIRTIO_F_SHM_PAGE_SIZE feature bit.
> 
> An implementation of the changes proposed in this series has been
> published as an RFC to the LKML, to be used as a reference:
> 
> https://lore.kernel.org/all/20250214-virtio-shm-page-size-v2-0-aa1619e6908b@redhat.com/
> 
> Sergio Lopez (3):
>   shared-mem: introduce page alignment restrictions
>   transport-pci: VIRTIO_F_SHM_PAGE_SIZE support
>   transport-mmio: VIRTIO_F_SHM_PAGE_SIZE
> 
>  content.tex        |  3 +++
>  shared-mem.tex     |  7 +++++++
>  transport-mmio.tex |  8 ++++++++
>  transport-pci.tex  | 10 +++++++++-
>  4 files changed, 27 insertions(+), 1 deletion(-)
> 

Looks good

Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>


-- 
Best regards,
Dmitry

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

[Parav Pandit]

> 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.

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

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

[Sergio Lopez Pascual]

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.

>>  \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.

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?

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.

Thanks!
Sergio.

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

[Michael S. Tsirkin]

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.

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

[Michael S. Tsirkin]

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


We can argue about new capabilities, but this is not doing it - it is
merely extending the existing one.

So I do not really get any of these arguments.


I'd be open to an alternative way to discover capabilities, as long as
we don't do that, I don't really see a reason to block minor tweaks
like this one.

At the same time, I am not sure why this is a capability at all.
why not in config space?

-- 
MST

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, March 4, 2025 4:23 PM
> 
> On Sun, Feb 23, 2025 at 05:21:00AM +0000, Parav Pandit wrote:
> > 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."
> 
> 
> We can argue about new capabilities, but this is not doing it - it is merely
> extending the existing one.
> 
This patch is not even extending it. it is reusing the pad field, which is good.
Extending capability is problematic.

> So I do not really get any of these arguments.
>
I asked him to explore what all options did he consider.
 
> 
> I'd be open to an alternative way to discover capabilities, as long as we don't
> do that, I don't really see a reason to block minor tweaks like this one.
> 
I am not blocking it. would like to know what other options were considered if the shared memory is going to grow or its just one-off field that can live in PCI capability field.
I suggest, that this should be done without negotiating a feature bit. Because the device is just telling the restrictions...

> At the same time, I am not sure why this is a capability at all.
> why not in config space?
> 
When you say "config space", you meant "virtio config space" and not "pci config space".
Assuming yes, I believe the problem is, Sergio needs to add this field in ALL 19+ devices config space, which is not good.

> --
> MST

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

[Michael S. Tsirkin]

On Tue, Mar 04, 2025 at 10:58:14AM +0000, Parav Pandit wrote:
> 
> 
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Sent: Tuesday, March 4, 2025 4:23 PM
> > 
> > On Sun, Feb 23, 2025 at 05:21:00AM +0000, Parav Pandit wrote:
> > > 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."
> > 
> > 
> > We can argue about new capabilities, but this is not doing it - it is merely
> > extending the existing one.
> > 
> This patch is not even extending it. it is reusing the pad field, which is good.
> Extending capability is problematic.
> 
> > So I do not really get any of these arguments.
> >
> I asked him to explore what all options did he consider.
>  
> > 
> > I'd be open to an alternative way to discover capabilities, as long as we don't
> > do that, I don't really see a reason to block minor tweaks like this one.
> > 
> I am not blocking it.

Sorry if it sounded I implied you do. I just said we shouldn't :)

> would like to know what other options were
> considered if the shared memory is going to grow or its just one-off
> field that can live in PCI capability field.
> I suggest, that this should be done without negotiating a feature bit. Because the device is just telling the restrictions...

True. But the feature is helpful for device to discover whether driver
follows them. If not it can fail FEATURES_OK if it wants to.

> > At the same time, I am not sure why this is a capability at all.
> > why not in config space?
> > 
> When you say "config space", you meant "virtio config space" and not "pci config space".
> Assuming yes, I believe the problem is, Sergio needs to add this field in ALL 19+ devices config space, which is not good.

Aha.

> > --
> > MST

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

[Parav Pandit]

> 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.

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

[Sergio Lopez Pascual]

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.

Thanks!
Sergio.

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

[Parav Pandit]

> 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.