Re: [PATCH v8 2/8] iommu/arm: Introduce iommu_add_dt_pci_sideband_ids API

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.02.2025 10:58, Mykyta Poturai wrote:
> On 10.02.25 12:52, Jan Beulich wrote:
>> On 10.02.2025 11:30, Mykyta Poturai wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -20,6 +20,7 @@
>>>   #include <xen/param.h>
>>>   #include <xen/softirq.h>
>>>   #include <xen/keyhandler.h>
>>> +#include <xen/acpi.h>
>>>   #include <xsm/xsm.h>
>>>   
>>>   #ifdef CONFIG_X86
>>> @@ -744,6 +745,18 @@ int __init 
>>> iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
>>>       return 0;
>>>   }
>>>   
>>> +int iommu_add_pci_sideband_ids(struct pci_dev *pdev)
>>> +{
>>> +    int ret = -EOPNOTSUPP;
>>> +
>>> +#ifdef CONFIG_HAS_PCI
>>> +    if ( acpi_disabled )
>>> +        ret = iommu_add_dt_pci_sideband_ids(pdev);
>>> +#endif
>>> +
>>> +    return ret;
>>> +}
>>
>> This function has no caller, which violates a Misra rule iirc. Considering
>> all information given here it's also unclear why it would gain a caller on
>> x86 (at least as long as DT isn't used there).
> 
> Would it be ok to wrap it with CONFIG_ARM? I am not quite sure how 
> relevant this mapping functionality is to X86 iommus, although Linux has 
> similar implementations for ACPI.

Besides it being unclear to me whether the function is really Arm-specific
(what about RISC-V or PPC), I also don't see how that would address the
Misra concern. (If the function was truly Arm-specific, it would better
move into an Arm-specific source file.)

> Alternatively, we can remove this abstraction for now, to call 
> iommu_add_dt_pci_sideband_ids from Arm directly and only introduce it 
> back when at least some ACPI implementation is done.

I'd leave that to Arm folks to judge.

> Also, just want to mention the issue that forced me to move this from 
> the header in the first place in case it is not known. There is a 
> conflict in fixed width integers definitions between actypes.h and 
> efibind.h and it was revealed when including acpi.h into iommu.h.
> I initially tried to fix the source of this conflict, but I don't know 
> enough about ACPI and EFI quirks to confidently do it.
> 
> In file included from ./include/acpi/acpi.h:57,
>                   from ./include/xen/acpi.h:57,
>                   from ./include/xen/iommu.h:28,
>                   from ./include/xen/sched.h:12,
>                   from ./arch/x86/include/asm/paging.h:17,
>                   from ./arch/x86/include/asm/guest_access.h:11,
>                   from ./include/xen/guest_access.h:10,
>                   from arch/x86/efi/runtime.c:5:
> ./include/acpi/actypes.h:130:35: error: conflicting types for ‘UINT64’; 
> have ‘long long unsigned int’
>    130 | typedef COMPILER_DEPENDENT_UINT64 UINT64;
>        |                                   ^~~~~~
> In file included from ./arch/x86/include/asm/efibind.h:2,
>                   from ./common/efi/efi.h:1,
>                   from arch/x86/efi/runtime.c:1:
> ./arch/x86/include/asm/x86_64/efibind.h:89:20: note: previous 
> declaration of ‘UINT64’ with type ‘UINT64’ {aka ‘long unsigned int’}
>     89 | typedef uint64_t   UINT64;

Yeah, sadly ACPI and EFI headers (both imported from different origins)
aren't overly compatible with one another.

Jan