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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Jan,

> 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.

Thanks in advance.

Regards,
Rahul