Hi Shameer, Jonathan,
On 6/18/25 10:35 AM, Shameerali Kolothum Thodi 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,
sorry but I still fail to understand why we can't just check against
TYPE_PCI_HOST_BRIDGE for making sure the SMMU is attached to PXB or GPEX. What
does it fail to check? Why shall we care about PCI vs PCIe?
Thanks
Eric
>>
>> 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.
>
> Thanks,
> Shameer
>
