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

[Shameerali Kolothum Thodi via]

> -----Original Message-----
> From: Nicolin Chen <nicol...@nvidia.com>
> Sent: Tuesday, July 8, 2025 9:57 PM
> 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 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