On Wed, 18 Jun 2025 09:35:35 +0100
Shameerali Kolothum Thodi <shameerali.kolothum.th...@huawei.com> wrote:
> > -----Original Message-----
> > From: Jonathan Cameron <jonathan.came...@huawei.com>
> > Sent: Tuesday, June 17, 2025 5:53 PM
> > To: Eric Auger <eric.au...@redhat.com>
> > Cc: Shameerali Kolothum Thodi
> > <shameerali.kolothum.th...@huawei.com>; Linuxarm
> > <linux...@huawei.com>; qemu-...@nongnu.org; qemu-
> > de...@nongnu.org; peter.mayd...@linaro.org; j...@nvidia.com;
> > nicol...@nvidia.com; ddut...@redhat.com; berra...@redhat.com;
> > imamm...@redhat.com; nath...@nvidia.com; mo...@nvidia.com;
> > smost...@google.com; Wangzhou (B) <wangzh...@hisilicon.com>;
> > jiangkunkun <jiangkun...@huawei.com>; zhangfei....@linaro.org
> > Subject: Re: [PATCH v4 1/7] hw/arm/smmu-common: Check SMMU has PCIe
> > Root Complex association
> >
> > On Tue, 17 Jun 2025 09:49:54 +0200
> > Eric Auger <eric.au...@redhat.com> wrote:
> >
> > > On 6/16/25 12:20 PM, Jonathan Cameron wrote:
> > > > On Fri, 13 Jun 2025 15:44:43 +0100
> > > > Shameer Kolothum <shameerali.kolothum.th...@huawei.com> wrote:
> > > >
> > > >> Although this change does not affect functionality at present, it is
> > > > Patch title says PCIe. This check is vs PCI host bridge.
> > > >
> > > > No idea which one you wanted, but if it is PCIe needs to be
> > > > TYPC_PCIE_HOST_BRIDGE from pcie_host.h not the pci_host.h one
> > > > I think.
> > > I think we need TYPE_PCI_HOST_BRIDGE as we want to check against pxb
> > >
> > > pci-bridge/pci_expander_bridge.c: .parent =
> > TYPE_PCI_HOST_BRIDGE,
> >
> > Hmm. That's awkward and I'd forgotten that wrinkle.
> > Need a stronger test but which one? The PXB root bus has a parent of
> > TYPE_PCIE_BUS. Maybe we can check that?
>
> Ok. How about we do something like below?
>
>
> @@ -925,6 +926,7 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
> {
> SMMUState *s = ARM_SMMU(dev);
> SMMUBaseClass *sbc = ARM_SMMU_GET_CLASS(dev);
> + PCIBus *pci_bus = s->primary_bus;
> Error *local_err = NULL;
>
> sbc->parent_realize(dev, &local_err);
> @@ -937,10 +939,31 @@ static void smmu_base_realize(DeviceState *dev,
> Error **errp)
> g_free, g_free);
> s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
>
> - if (s->primary_bus) {
> - pci_setup_iommu(s->primary_bus, &smmu_ops, s);
> - } else {
> + if (!pci_bus) {
> error_setg(errp, "SMMU is not attached to any PCI bus!");
> + return;
> + }
> +
> + /*
> + * We only allow default PCIe Root Complex(pcie.0) or pxb-pcie based
> extra
> + * root complexes to be associated with SMMU.
> + */
> + if (pci_bus_is_express(pci_bus) && pci_bus_is_root(pci_bus) &&
> + object_dynamic_cast(OBJECT(pci_bus)->parent, TYPE_PCI_HOST_BRIDGE)) {
> + /*
> + * For pxb-pcie, parent_dev will be set. Make sure it is
> + * pxb-pcie indeed.
> + */
> + if (pci_bus->parent_dev) {
> + if (!object_dynamic_cast(OBJECT(pci_bus), "pxb-pcie-bus")) {
> + error_setg(errp, "SMMU is not attached to pxb-pcie bus!");
> + return;
> + }
> + }
> + pci_setup_iommu(pci_bus, &smmu_ops, s);
> + } else {
> + error_setg(errp, "SMMU should be attached to a default PCIe
> root complex"
> + "(pcie.0) or a pxb-pcie based root complex");
> }
> }
>
> Please let me know if this is good enough or not.
LGTM
>
> Thanks,
> Shameer
