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

[Nicolin Chen <nicolinc@nvidia.com>]

On Wed, Jul 09, 2025 at 08:08:49AM +0000, Shameerali Kolothum Thodi wrote:
> > 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?

Yes.

> Or you meant
> a corresponding free in case of failure here?

Yes. But I saw the other two g_hash_table_new_full calls were not
reverted in the exit path either. Then I saw smmu_base_reset_exit
does the clean up of those two but not this smmu_pcibus_by_busptr.

> 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.

Hmm?

We have these two types defined as two different strings, right?

#define TYPE_PCIE_BUS "PCIE"
#define TYPE_PXB_PCIE_BUS "pxb-pcie-bus"

So the first test is to make sure pci_bus string is "PCIE",
then the second one testing the same pci_bus string will
never be true?

> > 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. 

I see. That's clear now. I think it'd help by writing:
		/*
		 * While pcie.0 doesn't set the parent_dev, either pxb-pcie or
		 * pxb-cxl does. Re-test the type to make sure it is pxb-pcie.
		 */

Thanks
Nicolin