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

Sergio Lopez Pascual
2025-03-05T19:08:53Z

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.