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

[PATCH v2 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.

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/

v2:
 - Remove the VIRTIO_F_SHM_PAGE_SIZE feature bit, exposing page_shift
   in the transports unconditionally (thanks Parav Pandit).
 - Didn't pick up R-b due to the significant change between revisions.

Sergio Lopez (3):
  shared-mem: introduce page alignment restrictions
  transport-pci: introduce page_shift field for SHM
  transport-mmio: introduce SHMPageShift register

 shared-mem.tex     |  6 ++++++
 transport-mmio.tex |  8 ++++++++
 transport-pci.tex  | 10 +++++++++-
 3 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.49.0

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

[Sergio Lopez]

Add a subsection for page alignment restrictions.

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

diff --git a/shared-mem.tex b/shared-mem.tex
index 6e6f6c4..2021083 100644
--- a/shared-mem.tex
+++ b/shared-mem.tex
@@ -34,6 +34,12 @@ \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}
+
+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.49.0

[PATCH v2 2/3] transport-pci: introduce page_shift field for SHM

[Sergio Lopez]

Borrowing 8 bits from the 16 bit padding in virtio_pci_cap to hold
a page_shift field which is 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..efb174a 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 \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.49.0

[PATCH v2 3/3] transport-mmio: introduce SHMPageShift register

[Sergio Lopez]

Introduce the "SHMPageShift" register which, when read, returns the
page shift value that can be used to derive the page size value
supported by the shared memory regions in 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..042a82c 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}{%
+    Reading from this register returns the page shift derived from the supported
+    page size for the memory region selected by the \field{SHMSel} register. 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.49.0

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

[Michael S. Tsirkin]

On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> Add a subsection for page alignment restrictions.
> 
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
>  shared-mem.tex | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/shared-mem.tex b/shared-mem.tex
> index 6e6f6c4..2021083 100644
> --- a/shared-mem.tex
> +++ b/shared-mem.tex
> @@ -34,6 +34,12 @@ \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}
> +
> +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.
> +

will make existing drivers autmatically incompliant with new devices.

we generally avoid things like this.

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

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

[Sergio Lopez Pascual]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
>> Add a subsection for page alignment restrictions.
>>
>> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> ---
>>  shared-mem.tex | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/shared-mem.tex b/shared-mem.tex
>> index 6e6f6c4..2021083 100644
>> --- a/shared-mem.tex
>> +++ b/shared-mem.tex
>> @@ -34,6 +34,12 @@ \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}
>> +
>> +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.
>> +
>
> will make existing drivers autmatically incompliant with new devices.
>
> we generally avoid things like this.

This is the reason why in v1 this was gated behind
VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
understood that, at least for the PCI transport, it was preferred to
have the page_shift field unconditionally:

https://lore.kernel.org/virtio-comment/CY8PR12MB71950314C5683EB7CFF0815BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/

Did I misunderstand the conclusions?

Sergio.

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

[Michael S. Tsirkin]

On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> >> Add a subsection for page alignment restrictions.
> >>
> >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> >> ---
> >>  shared-mem.tex | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/shared-mem.tex b/shared-mem.tex
> >> index 6e6f6c4..2021083 100644
> >> --- a/shared-mem.tex
> >> +++ b/shared-mem.tex
> >> @@ -34,6 +34,12 @@ \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}
> >> +
> >> +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.
> >> +
> >
> > will make existing drivers autmatically incompliant with new devices.
> >
> > we generally avoid things like this.
> 
> This is the reason why in v1 this was gated behind
> VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
> understood that, at least for the PCI transport, it was preferred to
> have the page_shift field unconditionally:
> 
> https://lore.kernel.org/virtio-comment/CY8PR12MB71950314C5683EB7CFF0815BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
> 
> Did I misunderstand the conclusions?
> 
> Sergio.

I think Parav missed this compatibility implication when he said:

	So to obtain a free information, feature bit is not necessary, this can be self-
	described in the pci capability itself.

Parav, this is about the MUST requirement referring to honoring
alignment, not about obtaining the information which indeed can
be done without.


-- 
MST

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, April 1, 2025 2:45 PM
> 
> On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> > > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> > >> Add a subsection for page alignment restrictions.
> > >>
> > >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> > >> ---
> > >>  shared-mem.tex | 6 ++++++
> > >>  1 file changed, 6 insertions(+)
> > >>
> > >> diff --git a/shared-mem.tex b/shared-mem.tex index 6e6f6c4..2021083
> > >> 100644
> > >> --- a/shared-mem.tex
> > >> +++ b/shared-mem.tex
> > >> @@ -34,6 +34,12 @@ \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}
> > >> +
> > >> +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.
> > >> +
> > >
> > > will make existing drivers autmatically incompliant with new devices.
> > >
> > > we generally avoid things like this.
> >
> > This is the reason why in v1 this was gated behind
> > VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
> > understood that, at least for the PCI transport, it was preferred to
> > have the page_shift field unconditionally:
> >
> > https://lore.kernel.org/virtio-
> comment/CY8PR12MB71950314C5683EB7CFF081
> > 5BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
> >
> > Did I misunderstand the conclusions?
> >
> > Sergio.
> 
> I think Parav missed this compatibility implication when he said:
> 
> 	So to obtain a free information, feature bit is not necessary, this can
> be self-
> 	described in the pci capability itself.
> 
> Parav, this is about the MUST requirement referring to honoring alignment,
> not about obtaining the information which indeed can be done without.
> 
It can't be MUST requirement as it breaks the driver which will not negotiate this feature bit.
So my understanding is, the device indicates one or multiple alignments it supports and driver can choose which alignment it wants to use to get better performance. 

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

[Sergio Lopez Pascual]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>> > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
>> >> Add a subsection for page alignment restrictions.
>> >>
>> >> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> >> ---
>> >>  shared-mem.tex | 6 ++++++
>> >>  1 file changed, 6 insertions(+)
>> >>
>> >> diff --git a/shared-mem.tex b/shared-mem.tex
>> >> index 6e6f6c4..2021083 100644
>> >> --- a/shared-mem.tex
>> >> +++ b/shared-mem.tex
>> >> @@ -34,6 +34,12 @@ \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}
>> >> +
>> >> +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.
>> >> +
>> >
>> > will make existing drivers autmatically incompliant with new devices.
>> >
>> > we generally avoid things like this.
>>
>> This is the reason why in v1 this was gated behind
>> VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
>> understood that, at least for the PCI transport, it was preferred to
>> have the page_shift field unconditionally:
>>
>> https://lore.kernel.org/virtio-comment/CY8PR12MB71950314C5683EB7CFF0815BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
>>
>> Did I misunderstand the conclusions?
>>
>> Sergio.
>
> I think Parav missed this compatibility implication when he said:
>
> 	So to obtain a free information, feature bit is not necessary, this can be self-
> 	described in the pci capability itself.
>
> Parav, this is about the MUST requirement referring to honoring
> alignment, not about obtaining the information which indeed can
> be done without.

Would it be reasonable to expose page_shift in the transports
unconditionally but only require drivers to honor the page alignment
restrictions if VIRTIO_F_SHM_PAGE_SIZE has been negotiated?

Personally, from a device/driver implementator PoV, I'd prefer to have
everything (the `page_shift` field in PCI, the SHMPageShift register in
MMIO and the requirement to honor page alignment) gated behind
VIRTIO_F_SHM_PAGE_SIZE, but if there's a reason to prefer the approach
mentioned above, I'm fine with it.

Thanks,
Sergio.

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

[Sergio Lopez Pascual]

Parav Pandit <parav@nvidia.com> writes:

>> From: Michael S. Tsirkin <mst@redhat.com>
>> Sent: Tuesday, April 1, 2025 2:45 PM
>>
>> On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
>> > "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >
>> > > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
>> > >> Add a subsection for page alignment restrictions.
>> > >>
>> > >> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> > >> ---
>> > >>  shared-mem.tex | 6 ++++++
>> > >>  1 file changed, 6 insertions(+)
>> > >>
>> > >> diff --git a/shared-mem.tex b/shared-mem.tex index 6e6f6c4..2021083
>> > >> 100644
>> > >> --- a/shared-mem.tex
>> > >> +++ b/shared-mem.tex
>> > >> @@ -34,6 +34,12 @@ \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}
>> > >> +
>> > >> +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.
>> > >> +
>> > >
>> > > will make existing drivers autmatically incompliant with new devices.
>> > >
>> > > we generally avoid things like this.
>> >
>> > This is the reason why in v1 this was gated behind
>> > VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
>> > understood that, at least for the PCI transport, it was preferred to
>> > have the page_shift field unconditionally:
>> >
>> > https://lore.kernel.org/virtio-
>> comment/CY8PR12MB71950314C5683EB7CFF081
>> > 5BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
>> >
>> > Did I misunderstand the conclusions?
>> >
>> > Sergio.
>>
>> I think Parav missed this compatibility implication when he said:
>>
>> 	So to obtain a free information, feature bit is not necessary, this can
>> be self-
>> 	described in the pci capability itself.
>>
>> Parav, this is about the MUST requirement referring to honoring alignment,
>> not about obtaining the information which indeed can be done without.
>>
> It can't be MUST requirement as it breaks the driver which will not negotiate this feature bit.
> So my understanding is, the device indicates one or multiple alignments it supports and driver can choose which alignment it wants to use to get better performance.

Not exactly. The device indicates its supported page size (just one),
which implies the driver needs to honor the alignment restrictions
derived from that page size.

Just to clarify, this is not a performance improvement. Today, there are
devices (i.e. virtio-gpu) that doesn't work when the host (device) is
running with on a system with a bigger page size than the guest
(driver).

Thanks,
Sergio.

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

[Parav Pandit]

> From: Sergio Lopez Pascual <slp@redhat.com>
> Sent: Tuesday, April 1, 2025 3:04 PM
> 
> Parav Pandit <parav@nvidia.com> writes:
> 
> >> From: Michael S. Tsirkin <mst@redhat.com>
> >> Sent: Tuesday, April 1, 2025 2:45 PM
> >>
> >> On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
> >> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >> >
> >> > > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> >> > >> Add a subsection for page alignment restrictions.
> >> > >>
> >> > >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> >> > >> ---
> >> > >>  shared-mem.tex | 6 ++++++
> >> > >>  1 file changed, 6 insertions(+)
> >> > >>
> >> > >> diff --git a/shared-mem.tex b/shared-mem.tex index
> >> > >> 6e6f6c4..2021083
> >> > >> 100644
> >> > >> --- a/shared-mem.tex
> >> > >> +++ b/shared-mem.tex
> >> > >> @@ -34,6 +34,12 @@ \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}
> >> > >> +
> >> > >> +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.
> >> > >> +
> >> > >
> >> > > will make existing drivers autmatically incompliant with new devices.
> >> > >
> >> > > we generally avoid things like this.
> >> >
> >> > This is the reason why in v1 this was gated behind
> >> > VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
> >> > understood that, at least for the PCI transport, it was preferred
> >> > to have the page_shift field unconditionally:
> >> >
> >> > https://lore.kernel.org/virtio-
> >> comment/CY8PR12MB71950314C5683EB7CFF081
> >> > 5BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
> >> >
> >> > Did I misunderstand the conclusions?
> >> >
> >> > Sergio.
> >>
> >> I think Parav missed this compatibility implication when he said:
> >>
> >> 	So to obtain a free information, feature bit is not necessary, this
> >> can be self-
> >> 	described in the pci capability itself.
> >>
> >> Parav, this is about the MUST requirement referring to honoring
> >> alignment, not about obtaining the information which indeed can be done
> without.
> >>
> > It can't be MUST requirement as it breaks the driver which will not negotiate
> this feature bit.
> > So my understanding is, the device indicates one or multiple alignments it
> supports and driver can choose which alignment it wants to use to get better
> performance.
> 
> Not exactly. The device indicates its supported page size (just one), which
> implies the driver needs to honor the alignment restrictions derived from that
> page size.
> 
A device may not know on which OS flavor or which cpu flavor it may run.
So it is better for the device to expose multiple supported page sizes alignment and driver can choose one configured on the driver side.
Keep in mind that often device != host (device can be actual device too).

When the driver page size is bigger, it can always align to the smaller page size told by the device, no?

> Just to clarify, this is not a performance improvement. Today, there are
> devices (i.e. virtio-gpu) that doesn't work when the host (device) is running
> with on a system with a bigger page size than the guest (driver).
> 
> Thanks,
> Sergio.

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

[Michael S. Tsirkin]

On Tue, Apr 01, 2025 at 11:26:38AM +0200, Sergio Lopez Pascual wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
> >>
> >> > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> >> >> Add a subsection for page alignment restrictions.
> >> >>
> >> >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> >> >> ---
> >> >>  shared-mem.tex | 6 ++++++
> >> >>  1 file changed, 6 insertions(+)
> >> >>
> >> >> diff --git a/shared-mem.tex b/shared-mem.tex
> >> >> index 6e6f6c4..2021083 100644
> >> >> --- a/shared-mem.tex
> >> >> +++ b/shared-mem.tex
> >> >> @@ -34,6 +34,12 @@ \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}
> >> >> +
> >> >> +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.
> >> >> +
> >> >
> >> > will make existing drivers autmatically incompliant with new devices.
> >> >
> >> > we generally avoid things like this.
> >>
> >> This is the reason why in v1 this was gated behind
> >> VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
> >> understood that, at least for the PCI transport, it was preferred to
> >> have the page_shift field unconditionally:
> >>
> >> https://lore.kernel.org/virtio-comment/CY8PR12MB71950314C5683EB7CFF0815BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
> >>
> >> Did I misunderstand the conclusions?
> >>
> >> Sergio.
> >
> > I think Parav missed this compatibility implication when he said:
> >
> > 	So to obtain a free information, feature bit is not necessary, this can be self-
> > 	described in the pci capability itself.
> >
> > Parav, this is about the MUST requirement referring to honoring
> > alignment, not about obtaining the information which indeed can
> > be done without.
> 
> Would it be reasonable to expose page_shift in the transports
> unconditionally but only require drivers to honor the page alignment
> restrictions if VIRTIO_F_SHM_PAGE_SIZE has been negotiated?
> 
> Personally, from a device/driver implementator PoV, I'd prefer to have
> everything (the `page_shift` field in PCI, the SHMPageShift register in
> MMIO and the requirement to honor page alignment) gated behind
> VIRTIO_F_SHM_PAGE_SIZE, but if there's a reason to prefer the approach
> mentioned above, I'm fine with it.
> 
> Thanks,
> Sergio.

It's unclear when is it ok to access the field then, or what is the
value if accessed when feature bit is not negotiated. On this I think
I agree it is easier to just have it there unconditionally.

-- 
MST

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

[Michael S. Tsirkin]

On Tue, Apr 01, 2025 at 09:37:21AM +0000, Parav Pandit wrote:
> > From: Sergio Lopez Pascual <slp@redhat.com>
> > Sent: Tuesday, April 1, 2025 3:04 PM
> > 
> > Parav Pandit <parav@nvidia.com> writes:
> > 
> > >> From: Michael S. Tsirkin <mst@redhat.com>
> > >> Sent: Tuesday, April 1, 2025 2:45 PM
> > >>
> > >> On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
> > >> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > >> >
> > >> > > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> > >> > >> Add a subsection for page alignment restrictions.
> > >> > >>
> > >> > >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> > >> > >> ---
> > >> > >>  shared-mem.tex | 6 ++++++
> > >> > >>  1 file changed, 6 insertions(+)
> > >> > >>
> > >> > >> diff --git a/shared-mem.tex b/shared-mem.tex index
> > >> > >> 6e6f6c4..2021083
> > >> > >> 100644
> > >> > >> --- a/shared-mem.tex
> > >> > >> +++ b/shared-mem.tex
> > >> > >> @@ -34,6 +34,12 @@ \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}
> > >> > >> +
> > >> > >> +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.
> > >> > >> +
> > >> > >
> > >> > > will make existing drivers autmatically incompliant with new devices.
> > >> > >
> > >> > > we generally avoid things like this.
> > >> >
> > >> > This is the reason why in v1 this was gated behind
> > >> > VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
> > >> > understood that, at least for the PCI transport, it was preferred
> > >> > to have the page_shift field unconditionally:
> > >> >
> > >> > https://lore.kernel.org/virtio-
> > >> comment/CY8PR12MB71950314C5683EB7CFF081
> > >> > 5BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
> > >> >
> > >> > Did I misunderstand the conclusions?
> > >> >
> > >> > Sergio.
> > >>
> > >> I think Parav missed this compatibility implication when he said:
> > >>
> > >> 	So to obtain a free information, feature bit is not necessary, this
> > >> can be self-
> > >> 	described in the pci capability itself.
> > >>
> > >> Parav, this is about the MUST requirement referring to honoring
> > >> alignment, not about obtaining the information which indeed can be done
> > without.
> > >>
> > > It can't be MUST requirement as it breaks the driver which will not negotiate
> > this feature bit.
> > > So my understanding is, the device indicates one or multiple alignments it
> > supports and driver can choose which alignment it wants to use to get better
> > performance.
> > 
> > Not exactly. The device indicates its supported page size (just one), which
> > implies the driver needs to honor the alignment restrictions derived from that
> > page size.
> > 
> A device may not know on which OS flavor or which cpu flavor it may run.
> So it is better for the device to expose multiple supported page sizes alignment and driver can choose one configured on the driver side.

I doubt it's a thing though. Device has minimal alignment it can support
and that's it.  What are "multiple page sizes" and why would device
care?


> Keep in mind that often device != host (device can be actual device too).
> 
> When the driver page size is bigger, it can always align to the smaller page size told by the device, no?

What if it's smaller?


> > Just to clarify, this is not a performance improvement. Today, there are
> > devices (i.e. virtio-gpu) that doesn't work when the host (device) is running
> > with on a system with a bigger page size than the guest (driver).
> > 
> > Thanks,
> > Sergio.
> 

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

[Sergio Lopez Pascual]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Tue, Apr 01, 2025 at 11:26:38AM +0200, Sergio Lopez Pascual wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>
>> > On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual wrote:
>> >> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> >>
>> >> > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
>> >> >> Add a subsection for page alignment restrictions.
>> >> >>
>> >> >> Signed-off-by: Sergio Lopez <slp@redhat.com>
>> >> >> ---
>> >> >>  shared-mem.tex | 6 ++++++
>> >> >>  1 file changed, 6 insertions(+)
>> >> >>
>> >> >> diff --git a/shared-mem.tex b/shared-mem.tex
>> >> >> index 6e6f6c4..2021083 100644
>> >> >> --- a/shared-mem.tex
>> >> >> +++ b/shared-mem.tex
>> >> >> @@ -34,6 +34,12 @@ \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}
>> >> >> +
>> >> >> +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.
>> >> >> +
>> >> >
>> >> > will make existing drivers autmatically incompliant with new devices.
>> >> >
>> >> > we generally avoid things like this.
>> >>
>> >> This is the reason why in v1 this was gated behind
>> >> VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread, I
>> >> understood that, at least for the PCI transport, it was preferred to
>> >> have the page_shift field unconditionally:
>> >>
>> >> https://lore.kernel.org/virtio-comment/CY8PR12MB71950314C5683EB7CFF0815BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
>> >>
>> >> Did I misunderstand the conclusions?
>> >>
>> >> Sergio.
>> >
>> > I think Parav missed this compatibility implication when he said:
>> >
>> > 	So to obtain a free information, feature bit is not necessary, this can be self-
>> > 	described in the pci capability itself.
>> >
>> > Parav, this is about the MUST requirement referring to honoring
>> > alignment, not about obtaining the information which indeed can
>> > be done without.
>>
>> Would it be reasonable to expose page_shift in the transports
>> unconditionally but only require drivers to honor the page alignment
>> restrictions if VIRTIO_F_SHM_PAGE_SIZE has been negotiated?
>>
>> Personally, from a device/driver implementator PoV, I'd prefer to have
>> everything (the `page_shift` field in PCI, the SHMPageShift register in
>> MMIO and the requirement to honor page alignment) gated behind
>> VIRTIO_F_SHM_PAGE_SIZE, but if there's a reason to prefer the approach
>> mentioned above, I'm fine with it.
>>
>> Thanks,
>> Sergio.
>
> It's unclear when is it ok to access the field then, or what is the
> value if accessed when feature bit is not negotiated. On this I think
> I agree it is easier to just have it there unconditionally.

Got it. Let's do that in v3.

Thanks!
Sergio.

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, April 1, 2025 3:48 PM
> 
> On Tue, Apr 01, 2025 at 09:37:21AM +0000, Parav Pandit wrote:
> > > From: Sergio Lopez Pascual <slp@redhat.com>
> > > Sent: Tuesday, April 1, 2025 3:04 PM
> > >
> > > Parav Pandit <parav@nvidia.com> writes:
> > >
> > > >> From: Michael S. Tsirkin <mst@redhat.com>
> > > >> Sent: Tuesday, April 1, 2025 2:45 PM
> > > >>
> > > >> On Mon, Mar 31, 2025 at 09:40:37AM -0700, Sergio Lopez Pascual
> wrote:
> > > >> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> > > >> >
> > > >> > > On Mon, Mar 31, 2025 at 11:05:46AM -0400, Sergio Lopez wrote:
> > > >> > >> Add a subsection for page alignment restrictions.
> > > >> > >>
> > > >> > >> Signed-off-by: Sergio Lopez <slp@redhat.com>
> > > >> > >> ---
> > > >> > >>  shared-mem.tex | 6 ++++++
> > > >> > >>  1 file changed, 6 insertions(+)
> > > >> > >>
> > > >> > >> diff --git a/shared-mem.tex b/shared-mem.tex index
> > > >> > >> 6e6f6c4..2021083
> > > >> > >> 100644
> > > >> > >> --- a/shared-mem.tex
> > > >> > >> +++ b/shared-mem.tex
> > > >> > >> @@ -34,6 +34,12 @@ \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}
> > > >> > >> +
> > > >> > >> +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.
> > > >> > >> +
> > > >> > >
> > > >> > > will make existing drivers autmatically incompliant with new
> devices.
> > > >> > >
> > > >> > > we generally avoid things like this.
> > > >> >
> > > >> > This is the reason why in v1 this was gated behind
> > > >> > VIRTIO_F_SHM_PAGE_SIZE. But from the discussion on that thread,
> > > >> > I understood that, at least for the PCI transport, it was
> > > >> > preferred to have the page_shift field unconditionally:
> > > >> >
> > > >> > https://lore.kernel.org/virtio-
> > > >> comment/CY8PR12MB71950314C5683EB7CFF081
> > > >> > 5BDCCA2@CY8PR12MB7195.namprd12.prod.outlook.com/
> > > >> >
> > > >> > Did I misunderstand the conclusions?
> > > >> >
> > > >> > Sergio.
> > > >>
> > > >> I think Parav missed this compatibility implication when he said:
> > > >>
> > > >> 	So to obtain a free information, feature bit is not necessary,
> > > >> this can be self-
> > > >> 	described in the pci capability itself.
> > > >>
> > > >> Parav, this is about the MUST requirement referring to honoring
> > > >> alignment, not about obtaining the information which indeed can
> > > >> be done
> > > without.
> > > >>
> > > > It can't be MUST requirement as it breaks the driver which will
> > > > not negotiate
> > > this feature bit.
> > > > So my understanding is, the device indicates one or multiple
> > > > alignments it
> > > supports and driver can choose which alignment it wants to use to
> > > get better performance.
> > >
> > > Not exactly. The device indicates its supported page size (just
> > > one), which implies the driver needs to honor the alignment
> > > restrictions derived from that page size.
> > >
> > A device may not know on which OS flavor or which cpu flavor it may run.
> > So it is better for the device to expose multiple supported page sizes
> alignment and driver can choose one configured on the driver side.
> 
> I doubt it's a thing though. Device has minimal alignment it can support and
> that's it.  What are "multiple page sizes" and why would device care?
> 
Since Sergio posted v3, I replied to the latest patches.
I am bit confused on what exactly is the problem and how this solves the problem.
I asked him in v3.

> 
> > Keep in mind that often device != host (device can be actual device too).
> >
> > When the driver page size is bigger, it can always align to the smaller page
> size told by the device, no?
> 
> What if it's smaller?
> 
I assume you meant, driver page size is smaller than device.
If so, it can access the bigger pages of the device.

> 
> > > Just to clarify, this is not a performance improvement. Today, there
> > > are devices (i.e. virtio-gpu) that doesn't work when the host
> > > (device) is running with on a system with a bigger page size than the guest
> (driver).
> > >
> > > Thanks,
> > > Sergio.
> >