On 28.02.2025 12:31, Mykyta Poturai wrote:
>
>
> On 26.02.25 12:48, Jan Beulich wrote:
>> 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
>
> Hi everyone
> I searched for an appropriate place to put this function but I am a
> little stuck here:
> - Can't be put in the header as static inline because of header conflict.
> - Putting it in Arm only files or defines feels wrong because it is not
> in fact Arm-specific.
> - In iommu.c it can become dead code on some architectures.
> - Removing it and calling iommu_add_dt_pci_sideband_ids goes against the
> effort to make DT and ACPI able to co-exist.
>
> Could you please suggest a good way forward from here? I see two
> possible ones:
>
> 1. Fix the header conflict and return it to the header as static inline
> - best solution in my opinion
I'm very likely to say "no" to anything trying to go this route. The
headers in question want leaving alone as much as possible, to stay
close to their originals. The only acceptable approach there would be
to propose adjusting said originals.
> 2. Move to arm files it gains callers on other architectures.
I fear I don't understand this one.
Jan
> If we are going for the first approach maybe you can provide some
> pointers on how to better deal with this header conflict? Adding ifdef
> guards to the definitions? Renaming the types in one of them to
> something like EFI_UINT64 or ACPI_UINT64? Would a successful boot on the
> ACPI/EFI system be enough to confirm that I didn't break anything or
> will it require some more specific tests?
>
