pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
devices, add setting PCI_COMMAND_MEMORY for MMIO-based UART devices too.
Note the MMIO-based devices in practice need a "pci" sub-option,
otherwise a few parameters are not initialized (including bar_idx,
reg_shift, reg_width etc). The "pci" is not supposed to be used with
explicit BDF, so do not key setting PCI_COMMAND_MEMORY on explicit BDF
being set. Contrary to the IO-based UART, pci_serial_early_init() will
not attempt to set BAR0 address, even if user provided io_base manually
- in most cases, those are with an offest and the current cmdline syntax
doesn't allow expressing it. Due to this, enable PCI_COMMAND_MEMORY only
if uart->bar is already populated. In similar spirit, this patch does
not support setting BAR0 of the bridge.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
This fixes the issue I was talking about on #xendevel. Thanks Roger for
the hint.

Changes in v2:
 - check if uart->bar instead of uart->io_base
 - move it ahead of !uart->ps_bdf_enable return
 - expand commit message.
---
 xen/drivers/char/ns16550.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 1b21eb93c45f..34231dcb23ea 100644
--- 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);
+        return;
+    }
+
+    if ( !uart->ps_bdf_enable )
         return;
 
     if ( uart->pb_bdf_enable )
-- 
2.39.2

On 25.04.2023 16:39, Marek Marczykowski-Górecki wrote:
> pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> devices, add setting PCI_COMMAND_MEMORY for MMIO-based UART devices too.

This sentence is odd, as by its grammar it looks to describe the current
situation only. The respective sentence in v1 did not have this issue.

> --- 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);
> +        return;
> +    }
> +
> +    if ( !uart->ps_bdf_enable )
>          return;
>  
>      if ( uart->pb_bdf_enable )

While I did suggest using uart->bar, my implication was that the io_base
check would then remain in place. Otherwise, if I'm not mistaken, MMIO-
based devices not specified via "com<N>=...,pci" would then wrongly take
the I/O port path.

Furthermore - you can't use uart->bar alone here, can you? The field is
set equally for MMIO and port based cards in pci_uart_config().

Jan

On Tue, May 02, 2023 at 12:53:15PM +0200, Jan Beulich wrote:
> On 25.04.2023 16:39, Marek Marczykowski-Górecki wrote:
> > pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> > devices, add setting PCI_COMMAND_MEMORY for MMIO-based UART devices too.
> 
> This sentence is odd, as by its grammar it looks to describe the current
> situation only. The respective sentence in v1 did not have this issue.
> 
> > --- 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);
> > +        return;
> > +    }
> > +
> > +    if ( !uart->ps_bdf_enable )
> >          return;
> >  
> >      if ( uart->pb_bdf_enable )
> 
> While I did suggest using uart->bar, my implication was that the io_base
> check would then remain in place. Otherwise, if I'm not mistaken, MMIO-
> based devices not specified via "com<N>=...,pci" would then wrongly take
> the I/O port path.

I don't think MMIO-based devices specified manually have great chance to
work anyway (see the commit message), but indeed I shouldn't have broken
them even more.

> Furthermore - you can't use uart->bar alone here, can you? The field is
> set equally for MMIO and port based cards in pci_uart_config().

Right, I'll restore the io_base check.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

On Tue, Apr 25, 2023 at 04:39:02PM +0200, Marek Marczykowski-Górecki wrote:
> pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> devices, add setting PCI_COMMAND_MEMORY for MMIO-based UART devices too.
> Note the MMIO-based devices in practice need a "pci" sub-option,
> otherwise a few parameters are not initialized (including bar_idx,
> reg_shift, reg_width etc). The "pci" is not supposed to be used with
> explicit BDF, so do not key setting PCI_COMMAND_MEMORY on explicit BDF
> being set. Contrary to the IO-based UART, pci_serial_early_init() will
> not attempt to set BAR0 address, even if user provided io_base manually
> - in most cases, those are with an offest and the current cmdline syntax
> doesn't allow expressing it. Due to this, enable PCI_COMMAND_MEMORY only
> if uart->bar is already populated. In similar spirit, this patch does
> not support setting BAR0 of the bridge.

FWIW (not that should be done here) but I think we also want to
disable memory decoding in pci_uart_config() while sizing the BAR.

> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> This fixes the issue I was talking about on #xendevel. Thanks Roger for
> the hint.
> 
> Changes in v2:
>  - check if uart->bar instead of uart->io_base
>  - move it ahead of !uart->ps_bdf_enable return
>  - expand commit message.
> ---
>  xen/drivers/char/ns16550.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> index 1b21eb93c45f..34231dcb23ea 100644
> --- 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.

Thanks, Roger.

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

Jan

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?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

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