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

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

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