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
