Re: [PATCH v2] ns16550: enable memory decoding on MMIO-based PCI console card

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.04.2023 01:43, Marek Marczykowski-Górecki wrote:
> On Wed, Apr 26, 2023 at 10:24:28AM +0200, Jan Beulich wrote:
>> On 26.04.2023 09:48, Roger Pau Monné wrote:
>>> On Tue, Apr 25, 2023 at 04:39:02PM +0200, Marek Marczykowski-Górecki wrote:
>>>> --- a/xen/drivers/char/ns16550.c
>>>> +++ b/xen/drivers/char/ns16550.c
>>>> @@ -272,7 +272,15 @@ static int cf_check ns16550_getc(struct serial_port 
>>>> *port, char *pc)
>>>>  static void pci_serial_early_init(struct ns16550 *uart)
>>>>  {
>>>>  #ifdef NS16550_PCI
>>>> -    if ( !uart->ps_bdf_enable || uart->io_base >= 0x10000 )
>>>> +    if ( uart->bar )
>>>> +    {
>>>> +        pci_conf_write16(PCI_SBDF(0, uart->ps_bdf[0], uart->ps_bdf[1],
>>>> +                                  uart->ps_bdf[2]),
>>>> +                         PCI_COMMAND, PCI_COMMAND_MEMORY);
>>>
>>> Don't you want to read the current command register first and just
>>> or PCI_COMMAND_MEMORY?
>>>
>>> I see that for IO decoding we already do it this way, but I'm not sure
>>> whether it could cause issues down the road by unintentionally
>>> disabling command flags.
>>
>> Quite some time ago I asked myself the same question when seeing that
>> code, but I concluded that perhaps none of the bits are really sensible
>> to be set for a device as simple as a serial one. I'm actually thinking
>> that for such a device to be used during early boot, it might even be
>> helpful if bits like PARITY or SERR get cleared. (Of course if any of
>> that was really the intention of the change introducing that code, it
>> should have come with a code comment.)
> 
> I have mirrored the approach used for IO ports, with similar thinking,
> and I read the above as you agree. Does it mean this patch is okay,
> or you request some change here?

Sorry, I've yet to get to look at v2 itself. So far I've only looked at
Roger's response.

Jan