Re: [PATCH] ACPI: RIMT: Enable PCI ACS for root complexes mapped to an IOMMU

>> From: Fangyu Yu <[email protected]>
>>
>> Mirror the ACPI IORT solution used on arm64 (see iort_enable_acs())
>> by scanning the RIMT table for PCIe root complex nodes during
>> riscv_acpi_rimt_init(), which runs before ACPI PCI enumeration. If a
>> root complex node's ID mapping points at an IOMMU node, call
>> pci_request_acs() immediately, so pci_acs_enable is already set by
>> the time any PCI device is scanned and pci_enable_acs() runs.
>>
>> Only one call to pci_request_acs() is needed regardless of how many
>> root complexes are found, so track that with a static acs_enabled
>> flag, same as iort_enable_acs() does.
>>
>> Signed-off-by: Fangyu Yu <[email protected]>
>> ---
>>  drivers/acpi/riscv/rimt.c | 56 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/acpi/riscv/rimt.c b/drivers/acpi/riscv/rimt.c
>> index e4538fa6c2c8..f22e28c8211e 100644
>> --- a/drivers/acpi/riscv/rimt.c
>> +++ b/drivers/acpi/riscv/rimt.c
>> @@ -179,6 +179,42 @@ static struct acpi_rimt_node *rimt_scan_node(enum acpi_rimt_node_type type,
>>         return NULL;
>>  }
>>
>> +#ifdef CONFIG_PCI
>> +static void __init rimt_enable_acs(struct acpi_rimt_node *rimt_node)
>> +{
>> +       static bool acs_enabled __initdata;
>> +       struct acpi_rimt_pcie_rc *pci_rc;
>> +       struct acpi_rimt_id_mapping *map;
>> +       struct acpi_rimt_node *parent;
>> +       int i;
>> +
>> +       if (acs_enabled || rimt_node->type != ACPI_RIMT_NODE_TYPE_PCIE_ROOT_COMPLEX)
>> +               return;
>> +
>> +       pci_rc = (struct acpi_rimt_pcie_rc *)rimt_node->node_data;
>> +       if (!pci_rc->id_mapping_offset || !pci_rc->num_id_mappings)
>> +               return;
>> +
>> +       map = ACPI_ADD_PTR(struct acpi_rimt_id_mapping, rimt_node,
>> +                          pci_rc->id_mapping_offset);
>> +
>> +       for (i = 0; i < pci_rc->num_id_mappings; i++, map++) {
>> +               if (!map->dest_offset)
>> +                       continue;
>> +
>> +               parent = ACPI_ADD_PTR(struct acpi_rimt_node, rimt_table,
>> +                                     map->dest_offset);
>> +               if (parent->type == ACPI_RIMT_NODE_TYPE_IOMMU) {
>> +                       pci_request_acs();
>> +                       acs_enabled = true;
>> +                       return;
>> +               }
>> +       }
>> +}
>> +#else
>> +static inline void rimt_enable_acs(struct acpi_rimt_node *rimt_node) { }
>> +#endif
>> +
>>  /*
>>   * RISC-V supports IOMMU as a PCI device or a platform device.
>>   * When it is a platform device, there should be a namespace device as
>> @@ -509,7 +545,10 @@ int rimt_iommu_configure_id(struct device *dev, const u32 *id_in)
>>
>>  void __init riscv_acpi_rimt_init(void)
>>  {
>> +       struct acpi_rimt_node *rimt_node, *rimt_end;
>> +       struct acpi_table_rimt *rimt;
>>         acpi_status status;
>> +       int i;
>>
>>         /* rimt_table will be used at runtime after the rimt init,
>>          * so we don't need to call acpi_put_table() to release
>> @@ -525,4 +564,21 @@ void __init riscv_acpi_rimt_init(void)
>>
>>                 return;
>>         }
>> +
>> +       rimt = (struct acpi_table_rimt *)rimt_table;
>> +       rimt_node = ACPI_ADD_PTR(struct acpi_rimt_node, rimt,
>> +                                rimt->node_offset);
>> +       rimt_end = ACPI_ADD_PTR(struct acpi_rimt_node, rimt_table,
>> +                               rimt_table->length);
>> +
>> +       for (i = 0; i < rimt->num_nodes; i++) {
>> +               if (rimt_node >= rimt_end) {
>> +                       pr_err("RIMT node pointer overflows, bad table\n");
>> +                       return;
>> +               }
>> +
>> +               rimt_enable_acs(rimt_node);
>>
>NIT: Can we check here itself if it is a PCI RC node and then only
>call enable_acs()?
>

Agreed, I will add the PCI RC node check here and invoke enable_acs()
only for that node type.

Thanks,
Fangyu

>> +               rimt_node = ACPI_ADD_PTR(struct acpi_rimt_node, rimt_node,
>> +                                        rimt_node->length);
>> +       }
>>  }
>> --
>> 2.50.1
>>
>Otherwise, LGTM.
>
>Reviewed-by: Sunil V L <[email protected]>