Re: [PATCH v4] iommu/of: Fix pci_request_acs() before enumerating PCI devices

[Xingang Wang]

On 2021/6/5 3:04, Bjorn Helgaas wrote:
> [+cc John, who tested 6bf6c24720d3]
>
> On Fri, May 21, 2021 at 03:03:24AM +0000, Wang Xingang wrote:
> > From: Xingang Wang <wangxingang5@xxxxxxxxxx>
> >
> > When booting with devicetree, the pci_request_acs() is called after the
> > enumeration and initialization of PCI devices, thus the ACS is not
> > enabled. And ACS should be enabled when IOMMU is detected for the
> > PCI host bridge, so add check for IOMMU before probe of PCI host and call
> > pci_request_acs() to make sure ACS will be enabled when enumerating PCI
> > devices.
>
> I'm happy to apply this, but I'm a little puzzled about 6bf6c24720d3
> ("iommu/of: Request ACS from the PCI core when configuring IOMMU
> linkage").  It was tested and fixed a problem, but I don't understand
> how.
>
> 6bf6c24720d3 added the call to pci_request_acs() in
> of_iommu_configure() so it currently looks like this:
>
>    of_iommu_configure(dev, ...)
>    {
>      if (dev_is_pci(dev))
>        pci_request_acs();
>
> pci_request_acs() sets pci_acs_enable, which tells us to enable ACS
> when enumerating PCI devices in the future.  But we only call
> pci_request_acs() if we already *have* a PCI device.
>
> So maybe 6bf6c24720d3 fixed a problem for *some* PCI devices, but not
> all?  E.g., did we call of_iommu_configure() for one PCI device before
> enumerating the rest?
I test the kernel on an arm platform with qemu:

qemu-system-aarch64 \
 -cpu host \
 -kernel arch/arm64/boot/Image \
 -enable-kvm \
 -m 8G \
 -smp 2,sockets=2,cores=1,threads=1 	\
 -machine virt,kernel_irqchip=on,gic-version=3,iommu=smmuv3\
 -initrd rootfs.cpio.gz \
 -nographic \
 -append "rdinit=init console=ttyAMA0 earlycon=pl011,0x9000000 nokaslr" \
 -device pcie-root-port,port=0x1,chassis=1,id=pci.1,addr=0x8 \
 -netdev user,id=hostnet0 \
 -device 
virtio-net-pci,netdev=hostnet0,id=net0,mac=08:13:3a:5a:22:5b,bus=pci.1,addr=0x0 
\

And find that the of_iommu_configure is called after the enumeration
of the pcie-root-port. And this might only infect the first device, when 
enumerating
the rest devices, the pci_acs_enable has already be enabled.

But to make sure that the pci_acs_enable will always be set before all 
PCI devices,
it would be better to set it in initialization of PCI bridges.

Thanks

Xingang

> > Fixes: 6bf6c24720d33 ("iommu/of: Request ACS from the PCI core when
> > configuring IOMMU linkage")
> > Signed-off-by: Xingang Wang <wangxingang5@xxxxxxxxxx>
> > ---
> >   drivers/iommu/of_iommu.c | 1 -
> >   drivers/pci/of.c         | 8 +++++++-
> >   2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/iommu/of_iommu.c b/drivers/iommu/of_iommu.c
> > index a9d2df001149..54a14da242cc 100644
> > --- a/drivers/iommu/of_iommu.c
> > +++ b/drivers/iommu/of_iommu.c
> > @@ -205,7 +205,6 @@ const struct iommu_ops *of_iommu_configure(struct device *dev,
> >   			.np = master_np,
> >   		};
> >   
> > -		pci_request_acs();
> >   		err = pci_for_each_dma_alias(to_pci_dev(dev),
> >   					     of_pci_iommu_init, &info);
> >   	} else {
> > diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> > index da5b414d585a..2313c3f848b0 100644
> > --- a/drivers/pci/of.c
> > +++ b/drivers/pci/of.c
> > @@ -581,9 +581,15 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
> >   
> >   int devm_of_pci_bridge_init(struct device *dev, struct pci_host_bridge *bridge)
> >   {
> > -	if (!dev->of_node)
> > +	struct device_node *node = dev->of_node;
> > +
> > +	if (!node)
> >   		return 0;
> >   
> > +	/* Detect IOMMU and make sure ACS will be enabled */
> > +	if (of_property_read_bool(node, "iommu-map"))
> > +		pci_request_acs();
> > +
> >   	bridge->swizzle_irq = pci_common_swizzle;
> >   	bridge->map_irq = of_irq_parse_and_map_pci;
> >   
> > --
> > 2.19.1
> .

.