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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello,

> On 19 Nov 2020, at 10:16 am, Julien Grall <julien@xxxxxxx> 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.  


> Cheers,
> 
> -- 
> Julien Grall


Regards,
Rahul