pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
devices, do similar with PCI_COMMAND_MEMORY for MMIO-based UART devices.

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.
---
 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..acfcce1c5d72 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -272,9 +272,17 @@ 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->ps_bdf_enable )
         return;
 
+    if ( uart->io_base >= 0x10000 )
+    {
+        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->pb_bdf_enable )
         pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
                                   uart->pb_bdf[2]),
-- 
2.39.2

On 25.04.2023 01:39, Marek Marczykowski-Górecki wrote:
> pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> devices, do similar with PCI_COMMAND_MEMORY for MMIO-based UART devices.

While I agree that something like this is needed (and in fact I have been
wondering more than once why this wasn't there), what you say above isn't
really correct imo: You do not really make things similar to port-based
handling. For one you don't respect uart->pb_bdf_enable. And then you also
don't write the BAR. When you use the BDF form of com<N>=, I don't see how
else the BAR could be written to the value that you (necessarily) have to
also specify on the command line. As said on Matrix, using the "pci"
sub-option together with the BDF one isn't intended (and would probably
better be rejected), according to all I can tell. Which in turn means that
for the "pci" sub-option alone to also have the effect of - if necessary -
enabling I/O or memory decoding, a further adjustment would be needed
(because merely keying this to uart->ps_bdf_enable then isn't enough). I
guess like e.g. ns16550_init_postirq() you'd want to also check uart->bar.

That said, I'm not meaning to mandate you to particularly deal with the
bridge part of the setup, not the least because I consider bogus what we
have. But you should at least mention in the description what you leave
untouched (and hence dissimilar to port-based handling).

As to rejecting invalid combinations of sub-options: See e.g. the dev_set
variable in parse_namevalue_pairs(). That's a wee attempt to go in the
intended direction.

Jan

> --- a/xen/drivers/char/ns16550.c
> +++ b/xen/drivers/char/ns16550.c
> @@ -272,9 +272,17 @@ 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->ps_bdf_enable )
>          return;
>  
> +    if ( uart->io_base >= 0x10000 )
> +    {
> +        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->pb_bdf_enable )
>          pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
>                                    uart->pb_bdf[2]),

On Tue, Apr 25, 2023 at 10:05:25AM +0200, Jan Beulich wrote:
> On 25.04.2023 01:39, Marek Marczykowski-Górecki wrote:
> > pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> > devices, do similar with PCI_COMMAND_MEMORY for MMIO-based UART devices.
> 
> While I agree that something like this is needed (and in fact I have been
> wondering more than once why this wasn't there), what you say above isn't
> really correct imo: You do not really make things similar to port-based
> handling. For one you don't respect uart->pb_bdf_enable. And then you also
> don't write the BAR. When you use the BDF form of com<N>=, I don't see how
> else the BAR could be written to the value that you (necessarily) have to
> also specify on the command line. 

I don't think MMIO-based UART is going to work without "pci" on the
command line at all. Setting the BAR is one of the reasons (there is
more to it than just setting (or reading) PCI_BASE_ADDRESS_0, as many
cards have UART registers at an offset), but also other parameters like
fifo_size. So, I don't think it's a good idea to set PCI_BASE_ADDRESS_0
to what user provided in io_base.

> As said on Matrix, using the "pci"
> sub-option together with the BDF one isn't intended (and would probably
> better be rejected), according to all I can tell. Which in turn means that
> for the "pci" sub-option alone to also have the effect of - if necessary -
> enabling I/O or memory decoding, a further adjustment would be needed
> (because merely keying this to uart->ps_bdf_enable then isn't enough). I
> guess like e.g. ns16550_init_postirq() you'd want to also check uart->bar.

Yes, checking also uart->bar makes sense.

> That said, I'm not meaning to mandate you to particularly deal with the
> bridge part of the setup, not the least because I consider bogus what we
> have. But you should at least mention in the description what you leave
> untouched (and hence dissimilar to port-based handling).
> 
> As to rejecting invalid combinations of sub-options: See e.g. the dev_set
> variable in parse_namevalue_pairs(). That's a wee attempt to go in the
> intended direction.

That makes sense with the current code shape. At some point IMO it's
worth having an option to choose which PCI device to use, also for
MMIO-based cards, but I don't have a need for this feature right now.

> Jan
> 
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -272,9 +272,17 @@ 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->ps_bdf_enable )
> >          return;
> >  
> > +    if ( uart->io_base >= 0x10000 )
> > +    {
> > +        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->pb_bdf_enable )
> >          pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
> >                                    uart->pb_bdf[2]),
> 

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

On Tue, Apr 25, 2023 at 10:05:25AM +0200, Jan Beulich wrote:
> On 25.04.2023 01:39, Marek Marczykowski-Górecki wrote:
> > pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
> > devices, do similar with PCI_COMMAND_MEMORY for MMIO-based UART devices.
> 
> While I agree that something like this is needed (and in fact I have been
> wondering more than once why this wasn't there), what you say above isn't
> really correct imo: You do not really make things similar to port-based
> handling. For one you don't respect uart->pb_bdf_enable. And then you also
> don't write the BAR. When you use the BDF form of com<N>=, I don't see how
> else the BAR could be written to the value that you (necessarily) have to
> also specify on the command line. 

I don't think MMIO-based UART is going to work without "pci" on the
command line at all. Setting the BAR is one of the reasons (there is
more to it than just setting (or reading) PCI_BASE_ADDRESS_0, as many
cards have UART registers at an offset), but also other parameters like
fifo_size. So, I don't think it's a good idea to set PCI_BASE_ADDRESS_0
to what user provided in io_base.

> As said on Matrix, using the "pci"
> sub-option together with the BDF one isn't intended (and would probably
> better be rejected), according to all I can tell. Which in turn means that
> for the "pci" sub-option alone to also have the effect of - if necessary -
> enabling I/O or memory decoding, a further adjustment would be needed
> (because merely keying this to uart->ps_bdf_enable then isn't enough). I
> guess like e.g. ns16550_init_postirq() you'd want to also check uart->bar.

Yes, checking also uart->bar makes sense.

> That said, I'm not meaning to mandate you to particularly deal with the
> bridge part of the setup, not the least because I consider bogus what we
> have. But you should at least mention in the description what you leave
> untouched (and hence dissimilar to port-based handling).
> 
> As to rejecting invalid combinations of sub-options: See e.g. the dev_set
> variable in parse_namevalue_pairs(). That's a wee attempt to go in the
> intended direction.

That makes sense with the current code shape. At some point IMO it's
worth having an option to choose which PCI device to use, also for
MMIO-based cards, but I don't have a need for this feature right now.

> Jan
> 
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -272,9 +272,17 @@ 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->ps_bdf_enable )
> >          return;
> >  
> > +    if ( uart->io_base >= 0x10000 )
> > +    {
> > +        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->pb_bdf_enable )
> >          pci_conf_write16(PCI_SBDF(0, uart->pb_bdf[0], uart->pb_bdf[1],
> >                                    uart->pb_bdf[2]),
> 

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

On 25.04.2023 11:22, Marek Marczykowski-Górecki wrote:
> On Tue, Apr 25, 2023 at 10:05:25AM +0200, Jan Beulich wrote:
>> On 25.04.2023 01:39, Marek Marczykowski-Górecki wrote:
>>> pci_serial_early_init() enables PCI_COMMAND_IO for IO-based UART
>>> devices, do similar with PCI_COMMAND_MEMORY for MMIO-based UART devices.
>>
>> While I agree that something like this is needed (and in fact I have been
>> wondering more than once why this wasn't there), what you say above isn't
>> really correct imo: You do not really make things similar to port-based
>> handling. For one you don't respect uart->pb_bdf_enable. And then you also
>> don't write the BAR. When you use the BDF form of com<N>=, I don't see how
>> else the BAR could be written to the value that you (necessarily) have to
>> also specify on the command line. 
> 
> I don't think MMIO-based UART is going to work without "pci" on the
> command line at all. Setting the BAR is one of the reasons (there is
> more to it than just setting (or reading) PCI_BASE_ADDRESS_0, as many
> cards have UART registers at an offset), but also other parameters like
> fifo_size. So, I don't think it's a good idea to set PCI_BASE_ADDRESS_0
> to what user provided in io_base.

While the BDF way of setting the device to use is meant for the most basic
configurations only anyway, I'm okay with you to leave out that aspect as
well, so long as you mention it as (another) dissimilarity with the port-
based logic.

>> As said on Matrix, using the "pci"
>> sub-option together with the BDF one isn't intended (and would probably
>> better be rejected), according to all I can tell. Which in turn means that
>> for the "pci" sub-option alone to also have the effect of - if necessary -
>> enabling I/O or memory decoding, a further adjustment would be needed
>> (because merely keying this to uart->ps_bdf_enable then isn't enough). I
>> guess like e.g. ns16550_init_postirq() you'd want to also check uart->bar.
> 
> Yes, checking also uart->bar makes sense.
> 
>> That said, I'm not meaning to mandate you to particularly deal with the
>> bridge part of the setup, not the least because I consider bogus what we
>> have. But you should at least mention in the description what you leave
>> untouched (and hence dissimilar to port-based handling).
>>
>> As to rejecting invalid combinations of sub-options: See e.g. the dev_set
>> variable in parse_namevalue_pairs(). That's a wee attempt to go in the
>> intended direction.
> 
> That makes sense with the current code shape. At some point IMO it's
> worth having an option to choose which PCI device to use, also for
> MMIO-based cards, but I don't have a need for this feature right now.

Well, in a very limited way this is already possible - see pci_uart_config()'s
"idx" parameter. The primary thing needed is extending ns16550_com[] to more
than two entries, or introducing a suitable level of indirection.

Jan