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

[Julien Grall <julien@xen.org>]

Hi Roger,

On 15/04/2021 14:26, Roger Pau Monné wrote:
> On Wed, Apr 14, 2021 at 09:49:37AM +0100, Julien Grall wrote:
>> 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).
> 
> Then I think some of the bits that are moved from
> xen/drivers/passthrough/pci.c (alloc_pci_msi, free_pci_msi and
> dump_pci_msi) need to be protected by a Kconfig option different than
> CONFIG_PCI_MSI_INTERCEPT, as those are not related to MSI interception,
> but to MSI handling in general (ie: Xen devices using MSI need
> those).

Well... x86-specific devices yes. We don't know in what form MSI will be 
added on for other arch-specific devices.

  The same with the msi_list and msix fields of pci_dev, those
> are not only used for MSI interception.
> 
> Or at least might be worth mentioning that the functions will be
> needed for Xen to be able to use MSI interrupts itself, even if not
> for interception purposes.

My preference would be to comment in the commit message (although, there 
is no promise they will ever get re-used). We can then modify the #ifdef 
once we introduce any use.

Cheers,

-- 
Julien Grall