| Message ID | 20230425143902.142571-1-marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ns16550: enable memory decoding on MMIO-based PCI console card | expand |
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?
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
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.
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 )
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(-)