Re: [PATCH] term/serial: Add support for PCI serial devices

[Glenn Washburn]

Hi Peter,

Thanks for the submission. I'm adding Daniel, the maintainer, because
otherwise he might not see it. Please TO or CC him on further
submissions.

On Wed, 24 Aug 2022 13:28:03 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> 
> Loosely based on early_pci_serial_init() from Linux, allow GRUB to make
> use of PCI serial devices.
> 
> Specifically, my Alderlake NUC exposes the Intel AMT SoL UART as a PCI
> enumerated device but doesn't include it in the EFI tables.
> 
> Tested and confirmed working on a "Lenovo P360 Tiny" with Intel AMT
> enabled. This specific machine has (from lspci -vv):
> 
> 00:16.3 Serial controller: Intel Corporation Device 7aeb (rev 11) (prog-if 02 
> [16550])
>         DeviceName: Onboard - Other
>         Subsystem: Lenovo Device 330e
>         Control: I/O+ Mem+ BusMaster- SpecCycle- MemWINV- VGASnoop- ParErr- 
> Stepping- SERR- FastB2B- DisINTx-
>         Status: Cap+ 66MHz+ UDF- FastB2B+ ParErr- DEVSEL=fast >TAbort- 
> <TAbort- <MAbort- >SERR- <PERR- INTx-
>         Interrupt: pin D routed to IRQ 19
>         Region 0: I/O ports at 40a0 [size=8]
>         Region 1: Memory at b4224000 (32-bit, non-prefetchable) [size=4K]
>         Capabilities: [40] MSI: Enable- Count=1/1 Maskable- 64bit+
>                 Address: 0000000000000000  Data: 0000
>         Capabilities: [50] Power Management version 3
>                 Flags: PMEClk- DSI+ D1- D2- AuxCurrent=0mA 
> PME(D0-,D1-,D2-,D3hot-,D3cold-)
>                 Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
>         Kernel driver in use: serial
> 
> From which the following config (/etc/default/grub) gets a working
> serial setup:
> 
> GRUB_CMDLINE_LINUX="console=tty0 earlyprintk=pciserial,00:16.3,115200 
> console=ttyS0,115200"
> GRUB_SERIAL_COMMAND="serial --port=0x40a0 --speed=115200"
> GRUB_TERMINAL="serial console"
> 
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  grub-core/term/serial.c |   59 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/grub/pci.h      |    3 ++
>  include/grub/serial.h   |    2 +
>  3 files changed, 64 insertions(+)
> 
> Index: grub2-2.06/grub-core/term/serial.c
> ===================================================================
> --- grub2-2.06.orig/grub-core/term/serial.c
> +++ grub2-2.06/grub-core/term/serial.c

It looks like this series is based of 2.06. I'm not sure if the patches
are identical when rebased on master, but you should make sure. Its
best practice to create patches off of master.

> @@ -34,6 +34,7 @@
>  #ifdef GRUB_MACHINE_IEEE1275
>  #include <grub/ieee1275/console.h>
>  #endif
> +#include <grub/pci.h>
>  
>  GRUB_MOD_LICENSE ("GPLv3+");
>  
> @@ -448,6 +449,7 @@ GRUB_MOD_INIT(serial)
>  #ifdef GRUB_MACHINE_ARC
>    grub_arcserial_init ();
>  #endif
> +  grub_pciserial_init ();
>  }
>  
>  GRUB_MOD_FINI(serial)
> @@ -461,3 +463,60 @@ GRUB_MOD_FINI(serial)
>      }
>    grub_unregister_extcmd (cmd);
>  }

This doesn't seem like the right place to put this code. Perhaps
grub-core/term/pci/serial.c is more consistent with other serial code.

> +
> +#define GRUB_SERIAL_PORT_NUM 4

Is 4 an arbitrary number? I would think we'd want to configure all
available. Is this to bound this in case something if off and we
indefinitely loop through PCI devices?

> +static char com_names[GRUB_SERIAL_PORT_NUM][20];
> +static struct grub_serial_port com_ports[GRUB_SERIAL_PORT_NUM];
> +static int ports = 0;

I think these globals would be better as static variables in
grub_pciserial_init() and passed in via the data argument in
find_pciserial().

> +
> +static int
> +find_pciserial (grub_pci_device_t dev, grub_pci_id_t pciid, void *data)
> +{
> +  grub_pci_address_t cmd_addr, class_addr, bar_addr;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  int err, i = ports;
> +
> +  (void)data;
> +  (void)pciid;
> +
> +  cmd_addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> +  cmdreg = grub_pci_read (cmd_addr);
> +
> +  class_addr = grub_pci_make_address (dev, GRUB_PCI_REG_REVISION);
> +  class = grub_pci_read (class_addr);
> +
> +  bar_addr = grub_pci_make_address (dev, GRUB_PCI_REG_ADDRESS_REG0);
> +  bar = grub_pci_read (bar_addr);
> +
> +  /* MODEM, SERIAL or MAGIC */

This comment is confusing to me. I think it would make more sense as
/* 16550 compatible MODEM or SERIAL */

> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +      ((class >> 8) & 0xff) != 0x02)

Perhaps a good macro name of 0x2 would be
GRUB_PCI_SERIAL_16550_COMPATIBLE, which appears to be the meaning of
the 0x2[1].

[1] https://wiki.osdev.org/PCI#Class_Codes

> +       return 0;
> +
> +  if (!(bar & 1)) /* Not I/O */

Would it be more appropriate to use GRUB_PCI_ADDR_SPACE_MASK here
instead of 1?

> +       return 0;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  grub_snprintf (com_names[i], sizeof (com_names[i]), "pci%d", i);
> +  com_ports[i].name = com_names[i];
> +  com_ports[i].driver = &grub_ns8250_driver;
> +  com_ports[i].port = bar & 0xfffffffc;

I think this could this be GRUB_PCI_ADDR_IO_MASK instead of 0xfffffffc.

> +  err = grub_serial_config_defaults (&com_ports[i]);
> +  if (err) {
> +       grub_print_error();

Missing space before '('.

> +       return 0;
> +  }
> +
> +  grub_serial_register (&com_ports[i]);
> +
> +  return ++ports >= GRUB_SERIAL_PORT_NUM;
> +}
> +
> +void
> +grub_pciserial_init (void)
> +{
> +     grub_pci_iterate (find_pciserial, NULL);
> +}
> Index: grub2-2.06/include/grub/serial.h
> ===================================================================
> --- grub2-2.06.orig/include/grub/serial.h
> +++ grub2-2.06/include/grub/serial.h
> @@ -193,6 +193,8 @@ const char *
>  grub_arcserial_add_port (const char *path);
>  #endif
>  
> +void grub_pciserial_init (void);
> +
>  struct grub_serial_port *grub_serial_find (const char *name);
>  extern struct grub_serial_driver grub_ns8250_driver;
>  void EXPORT_FUNC(grub_serial_unregister_driver) (struct grub_serial_driver 
> *driver);
> Index: grub2-2.06/include/grub/pci.h
> ===================================================================
> --- grub2-2.06.orig/include/grub/pci.h
> +++ grub2-2.06/include/grub/pci.h
> @@ -83,6 +83,9 @@
>  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
>  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03

These are inconsistent with the naming of the added macros below, but I
prefer the more informative naming style below. Would you be
interested in creating a second following patch in your v2 which
changes these above to GRUB_PCI_CLASS_DISPLAY_VGA and
GRUB_PCI_CLASS_SERIAL_USB?

>  
> +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> +
>  #ifndef ASM_FILE
>  
>  enum
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel