Re: [PATCH v3 1/3] xen/ns16550: Make ns16550 driver usable on ARM with HAS_PCI enabled.

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 19 Nov 2020, Julien Grall wrote:
> On Thu, 19 Nov 2020, 23:38 Stefano Stabellini, <sstabellini@xxxxxxxxxx> wrote:
>       On Thu, 19 Nov 2020, Rahul Singh wrote:
>       > > On 19/11/2020 09:53, Jan Beulich wrote:
>       > >> On 19.11.2020 10:21, Julien Grall wrote:
>       > >>> Hi Jan,
>       > >>>
>       > >>> On 19/11/2020 09:05, Jan Beulich wrote:
>       > >>>> On 18.11.2020 16:50, Julien Grall wrote:
>       > >>>>> On 16/11/2020 12:25, Rahul Singh wrote:
>       > >>>>>> NS16550 driver has PCI support that is under HAS_PCI flag. 
> When HAS_PCI
>       > >>>>>> is enabled for ARM, compilation error is observed for ARM 
> architecture
>       > >>>>>> because ARM platforms do not have full PCI support available.
>       > >>>>>   >
>       > >>>>>> Introducing new kconfig option CONFIG_HAS_NS16550_PCI to 
> support
>       > >>>>>> ns16550 PCI for X86.
>       > >>>>>>
>       > >>>>>> For X86 platforms it is enabled by default. For ARM platforms 
> it is
>       > >>>>>> disabled by default, once we have proper support for NS16550 
> PCI for
>       > >>>>>> ARM we can enable it.
>       > >>>>>>
>       > >>>>>> No functional change.
>       > >>>>>
>       > >>>>> NIT: I would say "No functional change intended" to make clear 
> this is
>       > >>>>> an expectation and hopefully will be correct :).
>       > >>>>>
>       > >>>>> Regarding the commit message itself, I would suggest the 
> following to
>       > >>>>> address Jan's concern:
>       > >>>>
>       > >>>> While indeed this is a much better description, I continue to 
> think
>       > >>>> that the proposed Kconfig option is undesirable to have.
>       > >>>
>       > >>> I am yet to see an argument into why we should keep the PCI code
>       > >>> compiled on Arm when there will be no-use....
>       > >> Well, see my patch suppressing building of quite a part of it.
>       > >
>       > > I will let Rahul figuring out whether your patch series is 
> sufficient to fix compilation issues (this is what matters right
>       now).
>       >
>       > I just checked the compilation error for ARM after enabling the 
> HAS_PCI on ARM. I am observing the same compilation error
>       what I observed previously.
>       > There are two new errors related to struct uart_config and struct 
> part_param as those struct defined globally but used under
>       X86 flags.
>       >
>       > At top level:
>       > ns16550.c:179:48: error: ‘uart_config’ defined but not used 
> [-Werror=unused-const-variable=]
>       >  static const struct ns16550_config __initconst uart_config[] =
>       >                                                 ^~~~~~~~~~~
>       > ns16550.c:104:54: error: ‘uart_param’ defined but not used 
> [-Werror=unused-const-variable=]
>       >  static const struct ns16550_config_param __initconst uart_param[] = {
>       >
>       >
>       > >
>       > >>>> Either,
>       > >>>> following the patch I've just sent, truly x86-specific things (at
>       > >>>> least as far as current state goes - if any of this was to be
>       > >>>> re-used by a future port, suitable further abstraction may be
>       > >>>> needed) should be guarded by CONFIG_X86 (or abstracted into arch
>       > >>>> hooks), or the HAS_PCI_MSI proposal would at least want further
>       > >>>> investigating as to its feasibility to address the issues at 
> hand.
>       > >>>
>       > >>> I would be happy with CONFIG_X86, despite the fact that this is 
> only
>       > >>> deferring the problem.
>       > >>>
>       > >>> Regarding HAS_PCI_MSI, I don't really see the point of 
> introducing given
>       > >>> that we are not going to use NS16550 PCI on Arm in the forseeable
>       > >>> future.
>       > >> And I continue to fail to see what would guarantee this: As soon
>       > >> as you can plug in such a card into an Arm system, people will
>       > >> want to be able use it. That's why we had to add support for it
>       > >> on x86, after all.
>       > >
>       > > Well, plug-in PCI cards on Arm has been available for quite a 
> while... Yet I haven't heard anyone asking for NS16550 PCI
>       support.
>       > >
>       > > This is probably because SBSA compliant server should always 
> provide an SBSA UART (a cut-down version of the PL011). So why
>       would bother to lose a PCI slot for yet another UART?
>       > >
>       > >> >> So why do we need a finer graine Kconfig?
>       > >> Because most of the involved code is indeed MSI-related?
>       > >
>       > > Possibly, yet it would not be necessary if we don't want NS16550 
> PCI support...
>       >
>       > To fix compilation error on ARM as per the discussion there are below 
> options please suggest which one to use to proceed
>       further.
>       >
>       > 1. Use the newly introduced CONFIG_HAS_NS16550_PCI config options. 
> This helps also non-x86 architecture in the future not to
>       have compilation error
>       > what we are observing now when HAS_PCI is enabled.
>       >
>       > 2. Guard the remaining x86 specific code with CONFIG_X86 and 
> introduce the new CONFIG_HAS_PCI_MSI options to fix the MSI
>       related compilation error.
>       > Once we have proper support for MSI and PCI for ARM  (HAS_PCI_MSI and 
> HAS_PCI enabled for ARM in Kconfig ) I am not sure if
>       NS16550 PCI will work out of the box on ARM .In that case, we might 
> need to come back again to fix NS16550 driver. 
> 
> 
>       It doesn't matter too much to me, let's just choose one option so that 
> you
>       get unblocked soon.
> 
>       It looks like Jan prefers option 2) and both Julien and I are OK with
>       it. So let's do 2). Jan, please confirm too :-)
> 
> 
> Please don't put words in my mouth... 

Sorry Julien, I misinterpreted one of your previous comments. Sometimes
it is difficult to do things by email. It is good that you clarified as
my goal was to reach an agreement.


> I think introducing HAS_PCI_MSI is short sighted.
> 
> There are no clear benefits of it when NS16550 PCI support is not going to be 
> enable in the foreseeable future.

I agree


> I would be ok with moving everything under CONFIG_X86. IHMO this is still 
> shortsighted but at least we don't introduce a config that's not
> going to help Arm or other any architecture to disable completely PCI support 
> in NS16550.

So you are suggesting a new option:

3. Guard the remaining x86 specific code *and* the MSI related
compilation errors with CONFIG_X86

Is that right?


My preference is actually option 1) but this series is already at v3 and
I don't think this decision is as important as much as unblocking
Rahul, so I am OK with the other alternatives too.

I tend to agree with you that 3) is better than 2) for the reasons you
wrote above.