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

[Jan Beulich <jbeulich@xxxxxxxx>]

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]),