Re: [PATCH v3 3/6] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 5/19/23 04:45, Jan Beulich wrote:
> On 18.05.2023 23:06, Stewart Hildebrand wrote:
>> --- a/xen/include/xen/iommu.h
>> +++ b/xen/include/xen/iommu.h
>> @@ -26,6 +26,7 @@
>>  #include <xen/spinlock.h>
>>  #include <public/domctl.h>
>>  #include <public/hvm/ioreq.h>
>> +#include <asm/acpi.h>
>>  #include <asm/device.h>
> 
> I view this as problematic: It'll require all architectures with an
> IOMMU implementation to have an asm/acpi.h. I think this wants to go
> inside an "#ifdef CONFIG_ACPI" and then ...

Will do

>> @@ -228,12 +230,25 @@ int iommu_release_dt_devices(struct domain *d);
>>   *      (IOMMU is not enabled/present or device is not connected to it).
>>   */
>>  int iommu_add_dt_device(struct dt_device_node *np);
>> +int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev);
>>
>>  int iommu_do_dt_domctl(struct xen_domctl *, struct domain *,
>>                         XEN_GUEST_HANDLE_PARAM(xen_domctl_t));
>>
>> +#else /* !HAS_DEVICE_TREE */
>> +static inline int iommu_add_dt_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    return 0;
>> +}
>>  #endif /* HAS_DEVICE_TREE */
>>
>> +static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
>> +{
>> +    if ( acpi_disabled )
> 
> ... the same #ifdef would be added around this if().

Okay. I will take care to avoid an unreachable return 0; by introducing a local 
variable:

static inline int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
{
    int ret = 0;
#ifdef CONFIG_ACPI
    if ( acpi_disabled )
#endif
        ret = iommu_add_dt_pci_sideband_ids(pdev);
    return ret;
}

> All of this of course only if this is deemed enough to allow co-existance
> of DT and ACPI (which I'm not convinced it is, but I don't know enough
> about DT and e.g. possible mixed configurations).
> 
> Jan

On ARM, we dynamically check for the existence of a valid device tree and set 
acpi_disabled accordingly. I did some basic testing on ARM with both 
CONFIG_ACPI=y and # CONFIG_ACPI is not set. My understanding is that it will 
work, and it should allow ACPI on ARM to be implemented in future.

>> +        return iommu_add_dt_pci_sideband_ids(pdev);
>> +    return 0;
>> +}
>> +
>>  struct page_info;
>>
>>  /*
>