> -----Original Message----- > From: Donald Dutile <[email protected]> > Sent: Thursday, July 10, 2025 5:00 PM > To: Shameerali Kolothum Thodi > <[email protected]>; Nicolin Chen > <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; [email protected]; > [email protected]; [email protected]; Linuxarm > <[email protected]>; Wangzhou (B) <[email protected]>; > jiangkunkun <[email protected]>; Jonathan Cameron > <[email protected]>; [email protected] > Subject: Re: [PATCH v7 07/12] hw/pci: Introduce > pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval > > > > On 7/10/25 3:37 AM, Shameerali Kolothum Thodi wrote: > > > > > >> -----Original Message----- > >> From: Nicolin Chen <[email protected]> > >> Sent: Thursday, July 10, 2025 1:07 AM > >> To: Shameerali Kolothum Thodi > <[email protected]> > >> Cc: [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected]; [email protected]; [email protected]; > >> [email protected]; [email protected]; > >> [email protected]; Linuxarm <[email protected]>; > Wangzhou > >> (B) <[email protected]>; jiangkunkun > <[email protected]>; > >> Jonathan Cameron <[email protected]>; > >> [email protected] > >> Subject: Re: [PATCH v7 07/12] hw/pci: Introduce > >> pci_setup_iommu_per_bus() for per-bus IOMMU ops retrieval > >> > >> On Wed, Jul 09, 2025 at 08:20:35AM +0000, Shameerali Kolothum Thodi > >> wrote: > >>>> On Tue, Jul 08, 2025 at 04:40:50PM +0100, Shameer Kolothum wrote: > >>>>> @@ -2909,6 +2909,19 @@ static void > >>>> pci_device_get_iommu_bus_devfn(PCIDevice *dev, > >>>>> } > >>>>> } > >>>>> > >>>>> + /* > >>>>> + * When multiple PCI Express Root Buses are defined using > >>>>> + pxb- > >>>> pcie, > >>>>> + * the IOMMU configuration may be specific to each root bus. > >>>> However, > >>>>> + * pxb-pcie acts as a special root complex whose parent > >>>>> + is > >>>> effectively > >>>>> + * the default root complex(pcie.0). Ensure that we retrieve > >>>>> the > >>>>> + * correct IOMMU ops(if any) in such cases. > >>>>> + */ > >>>>> + if (pci_bus_is_express(iommu_bus) && > >>>> pci_bus_is_root(iommu_bus)) { > >>>>> + if (!iommu_bus->iommu_per_bus && parent_bus- > >>>>> iommu_per_bus) { > >>>>> + break; > >>>> > >>>> Mind elaborating why the current bus must unset iommu_per_bus > >> while > >>>> its parent sets iommu_per_bus? > >>>> > >>>> My understanding is that for a pxb-pcie we should set > iommu_per_bus > >>>> but for its parent (the default root complex) we should unset its > >>>> iommu_per_bus? > >>> > >>> Well, for new arm-smmuv3 dev you need an associated pcie root > >>> complex. Either the default pcie.0 or a pxb-pcie one. And as I > >>> mentioned in my reply to the other thread(patch #2) and commit log > >> here, > >>> the pxb-pcie is special extra root complex in Qemu which has pcie.0 > >>> has parent bus. > >>> > >>> The above pci_device_get_iommu_bus_devfn() at present, iterate over > >> the > >>> parent_dev if it is set and returns the parent_bus IOMMU ops even if > >>> the associated pxb-pcie bus doesn't have any IOMMU. This creates > >>> problem for a case that is described here in the cover letter here, > >>> https://lore.kernel.org/qemu-devel/20250708154055.101012-1- > >> [email protected]/ > >>> > >>> (Please see "Major changes from v4:" section) > >>> > >>> To address that issue, this patch introduces an new helper function > >>> to > >> specify that > >>> the IOMMU ops are specific to the associated root > >> complex(iommu_per_bus) and > >>> use that to return the correct IOMMU ops. > >>> > >>> Hope with that context it is clear now. > >> > >> Hmm, I was not questioning the context, I get what the patch is > >> supposed to do. > >> > >> I was asking the logic that is unclear to me why it breaks when: > >> !pxb-pcie->iommu_per_bus && pcie.0->iommu_per_bus > >> > >> Or in which case pcie.0 would be set to iommu_per_bus=true? > > > > Yes. Consider the example I gave in cover letter, > > > > -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ -device > > virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \ -device > > pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \ -device > > arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \ -device > > pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \ -device > > virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1 > > > > pcie.0 is behind new SMMUv3 dev(smmuv3.1) and has iommu_per_bus > set. > > pcie.1 has no SMMv3U and iommu_per_bus is not set for it. > pcie.1 doesn't? then what is this line saying/meaning?: > -device arm-smmuv3,primary-bus=pcie.1,id=smmuv3.2 \ > > I read that as an smmuv3 attached to pcie.1, with an id of smmuv3.2; just > as I read this config: > -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ as an smmuv3 > attached to pcie.0 iwth id smmuv3.1
Oops..I forgot to delete that from the config: This is what I meant, -device arm-smmuv3,primary-bus=pcie.0,id=smmuv3.1 \ -device virtio-net-pci,bus=pcie.0,netdev=net0,id=virtionet.0 \ -device pxb-pcie,id=pcie.1,bus_nr=8,bus=pcie.0 \ -device pcie-root-port,id=pcie.port1,chassis=2,bus=pcie.1 \ -device virtio-net-pci,bus=pcie.port1,netdev=net1,id=virtionet.1 \ Thanks, Shameer > > > > > And we don't want pci_device_get_iommu_bus_devfn() to return pcie.0's > > IOMMU ops for virtionet.1. Hence the break. > > > > Thanks, > > Shameer > > >
