Re: [PATCH v2 2/6] hw/arm/virt-acpi-build: Update IORT for multiple smmuv3 devices

[Donald Dutile <ddutile@redhat.com>]

On 5/2/25 1:13 PM, Nicolin Chen wrote:
> On Fri, May 02, 2025 at 11:27:03AM +0100, Shameer Kolothum wrote:
>> @@ -43,6 +43,7 @@
>>   #include "hw/acpi/generic_event_device.h"
>>   #include "hw/acpi/tpm.h"
>>   #include "hw/acpi/hmat.h"
>> +#include "hw/arm/smmuv3.h"
>>   #include "hw/pci/pcie_host.h"
>>   #include "hw/pci/pci.h"
>>   #include "hw/pci/pci_bus.h"
>> @@ -266,6 +267,75 @@ static int iort_idmap_compare(gconstpointer a, gconstpointer b)
>>       return idmap_a->input_base - idmap_b->input_base;
>>   }
>>   
>> +struct SMMUv3Device {
>> +    int irq;
>> +    hwaddr base;
>> +    GArray *smmu_idmaps;
>> +    size_t offset;
>> +};
>> +typedef struct SMMUv3Device SMMUv3Device;
> 
> "SMMUv3Device" sounds too general, like coming from smmuv3.h :-/
> 
> Given this describes SMMUv3's IORT node, I still think we should
> name it something like "IortSMMUv3Node" or so.
> 
+1.. the more generic name had me thinking it was broader than IORT..
the IORT-related naming is an improvement.

>> +static int smmuv3_dev_idmap_compare(gconstpointer a, gconstpointer b)
>> +{
>> +    SMMUv3Device *sdev_a = (SMMUv3Device *)a;
>> +    SMMUv3Device *sdev_b = (SMMUv3Device *)b;
>> +    AcpiIortIdMapping *map_a = &g_array_index(sdev_a->smmu_idmaps,
>> +                                              AcpiIortIdMapping, 0);
>> +    AcpiIortIdMapping *map_b = &g_array_index(sdev_b->smmu_idmaps,
>> +                                              AcpiIortIdMapping, 0);
>> +    return map_a->input_base - map_b->input_base;
>> +}
>> +
>> +static void
>> +get_smmuv3_legacy_dev(VirtMachineState *vms, GArray * smmuv3_devices)
> 
> GArray *smmuv3_devices
> 
> Or maybe align with the non-legacy path, i.e. "sdev_blob"? Or the
> other way around.
> 
> Otherwise, lgtm
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
>