> -----Original Message-----
> From: Donald Dutile <ddut...@redhat.com>
> Sent: Thursday, July 10, 2025 5:00 PM
> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>; Nicolin Chen
> <nicol...@nvidia.com>
> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org;
> eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> berra...@redhat.com; imamm...@redhat.com; nath...@nvidia.com;
> mo...@nvidia.com; smost...@google.com; gustavo.rom...@linaro.org;
> m...@redhat.com; marcel.apfelb...@gmail.com; Linuxarm
> <linux...@huawei.com>; Wangzhou (B) <wangzh...@hisilicon.com>;
> jiangkunkun <jiangkun...@huawei.com>; Jonathan Cameron
> <jonathan.came...@huawei.com>; zhangfei....@linaro.org
> 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 <nicol...@nvidia.com>
> >> Sent: Thursday, July 10, 2025 1:07 AM
> >> To: Shameerali Kolothum Thodi
> <shameerali.kolothum.th...@huawei.com>
> >> Cc: qemu-...@nongnu.org; qemu-devel@nongnu.org;
> >> eric.au...@redhat.com; peter.mayd...@linaro.org; j...@nvidia.com;
> >> ddut...@redhat.com; berra...@redhat.com; imamm...@redhat.com;
> >> nath...@nvidia.com; mo...@nvidia.com; smost...@google.com;
> >> gustavo.rom...@linaro.org; m...@redhat.com;
> >> marcel.apfelb...@gmail.com; Linuxarm <linux...@huawei.com>;
> Wangzhou
> >> (B) <wangzh...@hisilicon.com>; jiangkunkun
> <jiangkun...@huawei.com>;
> >> Jonathan Cameron <jonathan.came...@huawei.com>;
> >> zhangfei....@linaro.org
> >> 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-
> >> shameerali.kolothum.th...@huawei.com/
> >>>
> >>> (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
> >
>
