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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 23 Nov 2020, Jan Beulich wrote:
> Rahul,
> 
> On 23.11.2020 12:54, Rahul Singh wrote:
> > Hello Jan,
> 
> as an aside - it helps if you also put the addressee of your mail
> on the To list.
> 
> >> On 20 Nov 2020, at 12:14 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> 
> >> 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.
> >>
> >>
> >>> 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.
> > 
> > 
> > Can you please provide your suggestion how to proceed on this so that I can 
> > send my next patch.
> > I am waiting for your reply if you are also ok for the options 3.
> 
> I can live with 3, I guess, but I still think a separate PCI_MSI
> control would be better. Please realize though that things also
> depend on how the change is going to look like in the end, i.e.
> I'm not going to assure you this is my final view on it. In any
> event I've just sent v2 of my series, which I consider a prereq
> of yours.

It is great that we have a way forward.

I'll try to have a look at your series -- it looks pretty
straightforward.