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 <pet...@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) <pet...@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 ;-)
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
