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

[Glenn Washburn]

On Wed, 24 Aug 2022 23:01:22 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

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

Nice day job, sincerely, we need more good folks crashing the kernel. :)

> > 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 :/

Yeah, it can get tricky. Its kind of custom build system layered on top
of autotools. The file you're looking for is
grub-core/Makefile.core.def.

Looking at this again, it looks like pci is only supported on x86 and
mips_loongson. So this is not as simple as it could be. I believe
you'll want to add the following lines to the serial module definition
(search for "name = serial;"):

  mips_loongson = term/pci/serial.c;
  x86 = term/pci/serial.c;

Also, the call to grub_pciserial_init() in grub-core/term/serial.c
should be ifdef'd to only be included for those platforms, otherwise
the other builds will break.

You will need to run the bootstrap script in the root of the repo to
rebuild the build scripts before running configure and make.

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

GRUB_SERIAL_PORT_NUM is only statically set to 4 when compiling for the
PCBIOS target, otherwise it varies depending on target. Since this code
is dynamically configuring serial ports based on what it finds via PCI,
why not just let it do all of them? There may be a good reason not to,
but this isn't my area of expertise so I don't know of one.

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

Its done that way in ns8250.c cause those globals are needed in
grub_serial_find() in grub-core/term/serial.c. I don't see this code
having those issues.

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

Yeah, I've grepped the source and its like than 10 places. And I think
there's no need for the blank line in the middle of the
GRUB_PCI_CLASS_* macros (the pre-existing ones and your new ones).

Glenn