On 20.03.2025 11:47, Mykyta Poturai wrote: > On 19.03.25 17:28, Jan Beulich wrote: >> On 19.03.2025 16:21, Mykyta Poturai wrote: >>> On 17.03.25 16:56, Jan Beulich wrote: >>>> On 14.03.2025 14:34, Mykyta Poturai wrote: >>>>> From: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com> >>>>> >>>>> The main purpose of this patch is to add a way to register PCI device >>>>> (which is behind the IOMMU) using the generic PCI-IOMMU DT bindings [1] >>>>> before assigning that device to a domain. >>>>> >>>>> This behaves similarly to the existing iommu_add_dt_device API, except it >>>>> handles PCI devices, and it is to be invoked from the add_device hook in >>>>> the >>>>> SMMU driver. >>>>> >>>>> The function dt_map_id to translate an ID through a downstream mapping >>>>> (which is also suitable for mapping Requester ID) was borrowed from Linux >>>>> (v5.10-rc6) and updated according to the Xen code base. >>>>> >>>>> [1] >>>>> https://www.kernel.org/doc/Documentation/devicetree/bindings/pci/pci-iommu.txt >>>>> >>>>> Signed-off-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com> >>>>> Signed-off-by: Stewart Hildebrand <stewart.hildebr...@amd.com> >>>>> Signed-off-by: Mykyta Poturai <mykyta_potu...@epam.com> >>>>> --- >>>>> Regarding pci_for_each_dma_alias question: getting host bridge node >>>>> directly seems like a simpler solution with the same result. AFAIU >>>>> with pci_for_each_dma_alias in linux we would arrive to the host brige >>>>> node anyway, but also try to call dt_map_id for each device along the >>>>> way. I am not sure why exactly it is done this way in linux, as >>>>> according to the pci-iommu.txt, iommu-map node can only be present in >>>>> the PCI root. >>>>> >>>>> v8->v9: >>>>> * replace DT_NO_IOMMU with 1 >>>>> * guard iommu_add_pci_sideband_ids with CONFIG_ARM >>>> >>>> I fear I'm confused: Isn't this contradicting ... >>>> >>>>> v7->v8: >>>>> * ENOSYS->EOPNOTSUPP >>>>> * move iommu_add_pci_sideband_ids to iommu.c to fix x86 build >>>> >>>> ... this earlier change? Really, with there being no caller, I can't see >>>> why there could be any build issue here affecting only x86. Except for >>>> Misra complaining about unreachable code being introduced, which I'm sure >>>> I said before should be avoided. >>> >>> The original reason for moving this function was the conflicting ACPI >>> and EFI headers, I described it in V8 comments here[1]. >>> >>>> >>>>> --- 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,20 @@ int __init >>>>> iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, >>>>> return 0; >>>>> } >>>>> >>>>> +#ifdef CONFIG_ARM >>>> >>>> I realize we have CONFIG_X86 here as well (visible even in context of the >>>> earlier hunk. Yet then the goal ought to be to reduce these anomalies, not >>>> add new ones. Since I don't have a clear picture of what's wanted, I'm also >>>> in trouble suggesting any alternative, I'm afraid. >>> >>> Here is a short summary: >>> >>> The main problem is that we need this function somewhere, but there is >>> no good place for it. It is only called on ARM for now but it's not >>> ARM-specific by nature and can be eventually used on other platforms as >>> well. It can't be just dropped because of the effort to support the >>> co-existence of DT and ACPI. It also can't be declared as a static >>> function because it requires the inclusion of <xen/acpi.h> for >>> acpi_disabled define, which leads to build errors[1]. And without ifdef >>> guards it would be a MISRA violation. >> >> An abridged version of this ought to go in the patch description, I think. >> This is special, so it needs calling out. >> >> As to the placement - would making an entirely new .c file possibly help? >> (Then, instead of in the patch description, maybe the special aspect could >> be put in a code comment at the top of the file.) > > It seems to me creating a new file would be overkill for one small > function. I considered moving it to xen/drivers/passthrough/arm/iommu.c > to reduce ifdefs but I feared it would suggest that it is arch-specific > a bit too strongly. So maybe move it there after all if you think it > would be better?
Well - with "#ifdef CONFIG_ARM" around it's Arm-specific too, isn't it? IOW - either have a proper (even if simple) abstraction, or perhaps indeed put it there (if that's also okay with the Arm maintainers). Jan
