RE: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has PCIe Root Complex association

[Shameerali Kolothum Thodi via <qemu-arm@nongnu.org>]

> -----Original Message-----
> From: Nicolin Chen <nicolinc@nvidia.com>
> Sent: Tuesday, July 8, 2025 9:57 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-arm@nongnu.org; qemu-devel@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; jgg@nvidia.com;
> ddutile@redhat.com; berrange@redhat.com; imammedo@redhat.com;
> nathanc@nvidia.com; mochs@nvidia.com; smostafa@google.com;
> gustavo.romero@linaro.org; mst@redhat.com;
> marcel.apfelbaum@gmail.com; Linuxarm <linuxarm@huawei.com>;
> Wangzhou (B) <wangzhou1@hisilicon.com>; jiangkunkun
> <jiangkunkun@huawei.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>; zhangfei.gao@linaro.org
> Subject: Re: [PATCH v7 02/12] hw/arm/smmu-common: Check SMMU has
> PCIe Root Complex association
> 
> On Tue, Jul 08, 2025 at 04:40:45PM +0100, Shameer Kolothum wrote:
> > @@ -937,11 +939,32 @@ static void smmu_base_realize(DeviceState
> *dev, Error **errp)
> >                                       g_free, g_free);
> >      s->smmu_pcibus_by_busptr = g_hash_table_new(NULL, NULL);
> 
> Although this is not introduced by this patch, is there a
> g_hash_table_remove() somewhere in the code?

g_hash_table_remove()  is to remove a key/value pair, isn't it? Or you meant
a corresponding free in case of failure here? It's a realize() fn and errp is set
if something goes wrong and QEMU will exit. Not sure we need an explicit
free here.
 
> > +    /*
> > +     * 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), TYPE_PXB_PCIE_BUS)) {
> 
> The pci_bus_is_express(pci_bus) at the top is equivalent to:
> 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PCIE_BUS)
> Then here it is doing:
> 	object_dynamic_cast(OBJECT(pci_bus), TYPE_PXB_PCIE_BUS)

Yes.

> So, this checks the same pci_bus but expects two different types?

In QEMU,  we can have three types of PCIe root complexes to be specified for
virt machine. 

1. default pcie.0 (TYPE_GPEX_HOST --> TYPE_PCIE_HOST_BRIDGE --> TYPE_PCI_HOST_BRIDGE)
2. pxb-pcie (TYPE_PXB_HOST  -->TYPE_PCI_HOST_BRIDGE)
2. pxb-cxl (TYPE_PXB_CXL_HOST  --> TYPE_PCI_HOST_BRIDGE)

The above first check is to see whether the bus is  PCIE && root bus && parent 
of type TYPE_PCI_HOST_BRIDGE. This will identify all the above three cases.

Both pxb-pcie and pxb-cxl are special extra root complexes based on PCI
expansion bridges and has a parent_dev set(both has pcie.0 has parent bus).

Hence we check to see parent_dev is set and make sure it is indeed 
TYPE_PXB_PCIE_BUS to avoid attaching to pxb-cxl. 

As mentioned in the commit log above, cxl support for virt is currently in
progress and once it has verified the functionality with SMMUv3
we can relax that check.

> I don't see the code check "PCIe Root Complex" explicitly, which
> should be TYPE_GPEX_HOST?

Hope it is clear now.

Thanks,
Shameer