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

[Peter Zijlstra]

On Wed, Aug 24, 2022 at 03:13:55PM -0500, Glenn Washburn wrote:
> 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.

Sure thing; not too familiar with the grub community.

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

Worse; it's based off of whatever: 'apt source grub2' got me for
debian testing.

I should indeed have checked out grub.git once it worked, but alas, I
was elated grub worked so I could get on with the day job of crashing
the kernel at will.

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

I did in fact first try to create term/pciserial.c but got lost in the
Makefile goo and gave up. Adding a file there is actually harder than
writing the patch :/

> > +
> > +#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?

It the same arbitrary number all the other serial code seems to use --
see for example ns8250.c; also I didn't try and figure out if there's
dynamic memory allocation available in grub.

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

This was modeled after the style of ns8250.c, but sure.

> > +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

Now you're going to make me clean up the Linux code I 'borrowed' that
thing from. That is indeed much better.

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

Indeed; I failed to check if there were appropriate defines for them.

> > +  err = grub_serial_config_defaults (&com_ports[i]);
> > +  if (err) {
> > +     grub_print_error();
> 
> Missing space before '('.

Argh, this coding style... :-)

> > +     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?

I supose I can do that, should be a fairly simple matter of running sed
over the output of git-grep I suppose ;-)