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: Thursday, July 10, 2025 12:54 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 02/12] hw/arm/smmu-common: Check SMMU has
> PCIe Root Complex association
> 
> 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.

Ok. I think that is by design. The insert for busptr cache happens during
early stages of Qemu through get_address_space() callback and
smmu_base_reset_exit() is called after that, just before the Guest boot.
So if you clean that cache at that time , you need to handle it differently
at a later stage. Also I don't think it makes much sense to clear busptr
before the Guest boot as it is not going to become stale unlike configs
and iotlb.

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

It will be true.

static const TypeInfo pxb_pcie_bus_info = {
    .name          = TYPE_PXB_PCIE_BUS,
    .parent        = TYPE_PCIE_BUS,
    .instance_size = sizeof(PXBBus),
    .class_init    = pxb_bus_class_init,
};

TYPE_PXB_PCIE_BUS has a parent TYPE_PCIE_BUS. And the function
object_dynamic_cast() does the magic. It will return non-null for an
exact object type and also for its parents in the QOM hierarchy.

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

I think it is already captured in the comments in this patch.

Thanks,
Shameer