Re: [PATCH v2] xen/pci: Refactor PCI MSI interrupts related code

[Julien Grall <julien@xen.org>]

Hi Roger,

On 14/04/2021 09:05, Roger Pau Monné wrote:
> On Tue, Apr 13, 2021 at 06:12:10PM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 12/04/2021 11:49, Roger Pau Monné wrote:
>>> On Fri, Apr 09, 2021 at 05:00:41PM +0100, Rahul Singh wrote:
>>>> diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
>>>> index 705137f8be..1b473502a1 100644
>>>> --- a/xen/drivers/passthrough/pci.c
>>>> +++ b/xen/drivers/passthrough/pci.c
>>>> @@ -1303,12 +1279,15 @@ static int __init setup_dump_pcidevs(void)
>>>>    }
>>>>    __initcall(setup_dump_pcidevs);
>>>> +
>>>> +#ifdef CONFIG_PCI_MSI_INTERCEPT
>>>>    int iommu_update_ire_from_msi(
>>>>        struct msi_desc *msi_desc, struct msi_msg *msg)
>>>>    {
>>>>        return iommu_intremap
>>>>               ? iommu_call(&iommu_ops, update_ire_from_msi, msi_desc, msg) : 0;
>>>>    }
>>>> +#endif
>>>
>>> This is not exactly related to MSI intercepts, the IOMMU interrupt
>>> remapping table will also be used for interrupt entries for devices
>>> being used by Xen directly, where no intercept is required.
>>
>> On Arm, this is not tie to the IOMMU. Instead, this handled is a separate
>> MSI controller (such as the ITS).
>>
>>>
>>> And then you also want to gate the hook from iommu_ops itself with
>>> CONFIG_PCI_MSI_INTERCEPT, if we want to got this route.
>>
>>
>> All the callers are in the x86 code. So how about moving the function in an
>> x86 specific file?
> 
> Yes, that seems fine. Just place it in asm-x86/iommu.h. I wonder why
> update_ire_from_msi wasn't moved when the rest of the x86 specific
> functions where moved there.

I am guessing it is because the function was protected by CONFIG_HAS_PCI 
rather than CONFIG_X86. So it was deferred until another arch enables 
CONFIG_HAS_PCI (it is easier to know what code should be moved).

> Was there an aim to use that in other
> arches?

In the future we may need to use MSIs in Xen (IIRC some SMMUs only 
support MSI interrupt). But I think the naming would be misleading as 
the IOMMU will not be used for the remapping.

So most likely, we would want a more generic name (maybe 
"arch_register_msi()"). This could call iommu_update_ire_from_msi() on 
x86 and the ITS on Arm.

I don't know how RISCv and PowerPC remap the interrupt. But if they are 
using the IOMMU then we could provide a generic helper protected by 
CONFIG_HAS_IOMMU_INTERRUPT_REMAP (or a similar name).

> 
> The hook in iommu_ops also need to be moved inside the x86 region.
> Please do this iommu change in a separate patch.

+1

Cheers,

-- 
Julien Grall