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

[Julien Grall <julien@xxxxxxx>]

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