Re: [PATCH V4] PCI: Extend ACS configurability
From: Steffen Dirkwinkel
Date: Mon Oct 07 2024 - 12:38:14 EST
On Tue, 2024-10-01 at 16:33 -0300, Jason Gunthorpe wrote:
> On Wed, Sep 25, 2024 at 07:49:59AM +0200, Jiri Slaby wrote:
> > On 25. 09. 24, 7:29, Jiri Slaby wrote:
> > > On 25. 09. 24, 7:06, Jiri Slaby wrote:
> > > > > @@ -1047,23 +1066,33 @@ static void pci_std_enable_acs(struct
> > > > > pci_dev *dev)
> > > > > */
> > > > > static void pci_enable_acs(struct pci_dev *dev)
> > > > > {
> > > > > - if (!pci_acs_enable)
> > > > > - goto disable_acs_redir;
> > > > > + struct pci_acs caps;
> > > > > + int pos;
> > > > > +
> > > > > + pos = dev->acs_cap;
> > > > > + if (!pos)
> > > > > + return;
> >
> > Ignore the previous post.
> >
> > The bridge has no ACS (see lspci below). So it used to be enabled
> > by
> > pci_quirk_enable_intel_pch_acs() by another registers.
>
> Er, Ok, so it overrides the whole thing with
> pci_dev_specific_acs_enabled() too..
>
> > I am not sure how to fix this as we cannot have "caps" from these
> > quirks, so
> > that whole idea of __pci_config_acs() is nonworking for these
> > quirks.
>
> We just need to allow the quirk to run before we try to do anything
> with the cap, which has probably always been a NOP anyhow.
>
> Maybe like this?
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 7d85c04fbba2ae..225a6cd2e9ca3b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1067,8 +1067,15 @@ static void pci_std_enable_acs(struct pci_dev
> *dev, struct pci_acs *caps)
> static void pci_enable_acs(struct pci_dev *dev)
> {
> struct pci_acs caps;
> + bool enable_acs = false;
> int pos;
>
> + /* If an iommu is present we start with kernel default caps
> */
> + if (pci_acs_enable) {
> + if (pci_dev_specific_enable_acs(dev))
> + enable_acs = true;
> + }
> +
> pos = dev->acs_cap;
> if (!pos)
> return;
> @@ -1077,11 +1084,8 @@ static void pci_enable_acs(struct pci_dev
> *dev)
> pci_read_config_word(dev, pos + PCI_ACS_CTRL, &caps.ctrl);
> caps.fw_ctrl = caps.ctrl;
>
> - /* If an iommu is present we start with kernel default caps
> */
> - if (pci_acs_enable) {
> - if (pci_dev_specific_enable_acs(dev))
> - pci_std_enable_acs(dev, &caps);
> - }
> + if (enable_acs)
> + pci_std_enable_acs(dev, &caps);
>
> /*
> * Always apply caps from the command line, even if there is
> no iommu.
Hi,
I just ran into this issue (fewer iommu groups starting with 6.11).
Both reverting the original patch or applying your suggestion worked
for me.
Thanks
Steffen