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

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

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