Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI

From: Julien Grall <julien@xen.org>
To: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Rahul Singh <rahul.singh@arm.com>,
	xen-devel@lists.xenproject.org, bertrand.marquis@arm.com,
	Jan Beulich <jbeulich@suse.com>, Paul Durrant <paul@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Ian Jackson <iwj@xenproject.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Daniel De Graaf <dgdegra@tycho.nsa.gov>
Subject: Re: [PATCH 2/2] xen/pci: Gate all MSI code in common code with CONFIG_HAS_PCI_MSI
Date: Tue, 6 Apr 2021 16:09:34 +0100	[thread overview]
Message-ID: <88704bcf-a06b-cf89-5fa3-0db94428f9f8@xen.org> (raw)
In-Reply-To: <YGx3TsTlAuE9eQ7i@Air-de-Roger>

On 06/04/2021 15:59, Roger Pau Monné wrote:
> On Tue, Apr 06, 2021 at 03:30:01PM +0100, Julien Grall wrote:
>> Hi Roger,
>>
>> On 06/04/2021 15:13, Roger Pau Monné wrote:
>>> On Tue, Apr 06, 2021 at 12:39:11PM +0100, Rahul Singh wrote:
>>>> MSI support is not implemented for ARM architecture but it is enabled
>>>> for x86 architecture and referenced in common passthrough/pci.c code.
>>>>
>>>> Therefore introducing the new flag to gate the MSI code for ARM in
>>>> common code to avoid compilation error when HAS_PCI is enabled for ARM.
>>>
>>> Is such option really interesting long term?
>>>
>>> IIRC PCI Express mandates MSI support, at which point I don't see much
>>> value in being able to compile out the MSI support.
>>
>> I am pretty sure there are board out with PCI support but no MSI support.
>> Anyway, even if the spec may mandate it...
>>
>>>
>>> So while maybe helpful for Arm PCI efforts ATM, I'm not sure it
>>> warrants a Kconfig option, I would rather see Arm introduce dummy
>>> helpers for the missing functionality, even if unimplemented at the
>>> moment.
>>
>> ... from my understanding, most of (if not all) the MSI code is not very
>> useful on Arm when using the GICv3 ITS.
>>
>> The GICv3 ITS will do the isolation for you and therefore we should not need
>> to keep track of the state at the vPCI level.
> 
> Right, but MSI has nothing to do with isolation, is just the
> capability to setup interrupts from PCI devices. What about systems
> without GICv3 ITS, is there an aim to support those also? (as from my
> reading of your reply those would require more auditing of the MSI
> accesses by the guests)

I am not aware of any plan for them so far.

> 
>> So I think we want to be able to compile out the code if not used. That
>> said, I think providing stub would be better to avoid multiple #ifdef in the
>> same function.
> 
> I think providing stubs is the way to go, that should allow to remove
> the unneeded code without having to explicitly drop MSI support. 
> As
> said before, I think it's fine to provide those unimplemented for Arm
> ATM, can be filled later if there's more pressing PCI work to do
> first.

We should remove unneeded and *avoid* allocation. Providing stub for 
existing functions will only address the first problem.

For the allocation (see alloc_pdev()) , we will need to move it in 
separate function and gate them to prevent the allocation.

It would be wrong to gate the code with #ifdef CONFIG_X86. So I think 
Rahul's idea to provide the new #ifdef is correct.

Cheers,

-- 
Julien Grall