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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 20/11/2020 00:14, Stefano Stabellini wrote:
> 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.
No worries. I would like to apologies for been harsher than I would have 
wanted on my reply. The thread had a lot of back and forth so far.
> > 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?
That's correct.

> 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.
In order, my preferences are 1) 3) 2). AFAICT...

> I tend to agree with you that 3) is better than 2) for the reasons you
> wrote above.
... this is the same order as you. Although I probably have a stronger 
dislike for 2) because I feel it is pushed for the wrong reasons (e.g. 
matter of taste) so far.
My view on 2) can change if Jan provides enough information into why one 
would want NS1650 PCI enabled by default on Arm but disable MSI.
Cheers,

--
Julien Grall