Re: [PATCH 03/11] iommu: Compute iommu_groups properly for PCIe switches

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 22 Sep 2025 22:26:26 -0400
Donald Dutile <ddutile@redhat.com> wrote:

> On 9/22/25 9:10 PM, Alex Williamson wrote:
> > On Mon, 22 Sep 2025 20:15:41 -0300
> > Jason Gunthorpe <jgg@nvidia.com> wrote:
> >   
> >> On Mon, Sep 22, 2025 at 04:32:00PM -0600, Alex Williamson wrote:  
> >>> The ACS capability was only introduced in PCIe 2.0 and vendors have
> >>> only become more diligent about implementing it as it's become
> >>> important for device isolation and assignment.  
> >>
> >> IDK about this, I have very new systems and they still not have ACS
> >> flags according to this interpretation.  
> > 
> > But how can we assume that lack of a non-required capability means
> > anything at all??
> >     
> ok, I'll bite on the the dumb answer...
> lots of non-support is represented by lack of a control structure.
> ... should we assume there are hidden VFs b/c there is a lack of a vf cap structure?
> ... <insert your favorite dumb answer here> :-)

This is not how an additive specification works.  We start with a base
specification.  We add capabilities to describe features of the device.
If a device doesn't support an SR-IOV capability, it doesn't support
VFs.  But likewise we cannot add an optional capability and
retroactively declare that anything that does not support this
capability must have some specific behavior.

That's not what the spec is doing.  We're misinterpreting it.  The
sections of the spec you're quoting are saying that if a MFD function
supports ACS it must support this specific p2p set of capability and
control bits unless the device does not support internal p2p.

> I can certainly see why a hw vendor would -not- put a control structure
> into a piece of hw that is not needed, as the spec states.
> For every piece of hw one creates, one has to invest resources to verify
> it is working correctly, and if verification is done correctly, verify it
> doesn't cause unexpected errors.  I've seen this resource req back in
> my HDL days (developers design w/HDL; hw verification engineers are the
> QE equivalent to sw, verifying the hw does and does not do what it is spec'd).

As previously noted, an "empty" ACS capability serves this purpose with
minimal verification.

> Penalizing a hw vendor for following the spec, and saving resources,
> seems wrong to me, to require them to quirk their spec-correct device.

IMO, we're clearly conflating the implementation of the ACS p2p
capability bits with the implementation of the ACS extended capability
itself.

> I suspect section 6.12.1.2 was written by hw vendors, looking to reduce
> their hw design & verification efforts.  If written by sw vendors, it
> would have likely required 'empty ACS' structs as you have mentioned in other thread(s).

We've had NIC vendors implement an empty ACS capability to convey the
fact that the device does not support internal p2p.  There is precedent
for the interpretation I'm describing.

> >>> IMO, we can't assume anything at all about a multifunction device
> >>> that does not implement ACS.  
> >>
> >> Yeah this is all true.
> >>
> >> But we are already assuming. Today we assume MFDs without caps must
> >> have internal loopback in some cases, and then in other cases we
> >> assume they don't.  
> > 
> > Where?  Is this in reference to our handling of multi-function
> > endpoints vs whether downstream switch ports are represented as
> > multi-function vs multi-slot?
> > 
> > I believe we consider multifunction endpoints and root ports to lack
> > isolation if they do not expose an ACS capability and an "empty" ACS
> > capability on a multifunction endpoint is sufficient to declare that
> > the device does not support internal p2p.  Everything else is quirks.
> >   
> >> I've sent and people have tested various different rules - please tell
> >> me what you can live with.  
> > 
> > I think this interpretation that lack of an ACS capability implies
> > anything is wrong.  Lack of a specific p2p capability within an ACS
> > capability does imply lack of p2p support.
> >   
> >> Assuming the MFD does not have internal loopback, while not entirely
> >> satisfactory, is the one that gives the least practical breakage.  
> > 
> > Seems like it's fixing one gap and opening another.  I don't see that we
> > can implement ingress and egress isolation without breakage.  We may
> > need an opt-in to continue egress only isolation.
> >   
> >> I think it most accurately reflects the majority of real hardware out
> >> there.
> >>
> >> We can quirk to fix the remainder.
> >>
> >> This is the best plan I've got..  
> > 
> > And hardware vendors are going to volunteer that they lack p2p
> > isolation and we need to add a quirk to reduce the isolation... the
> > dynamics are not in our favor.  Hardware vendors have no incentive to
> > do the right thing.  Thanks,
> >   
> I gave an example above why hw vendors have every incentive not to
> add an ACS structure if they don't need it. Not doing so, when they
> can do p2p, is a clear PCIe spec violation.  Punishing the correct
> implementations for the incorrect ones is not appropriate, and is
> further incentive to continue to be incorrect.
> 
> Don't we have the hooks with kernel cmdline disable_acs_redir &
> config_acs params to solve the insecure cases that may (would) be
> found, so breaking the isolation is relatively easy to fix vs adding
> quirks as is done today for proper spec interpretation?

Are we going to expect users to opt-in to securing their system?  This
is just doubling down on an incorrect spec interpretation.  Lack of an
optional extended capability cannot convey anything about the p2p
capabilities of the device <full stop>.  Thanks,

Alex