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

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

[Peter Zijlstra]

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
@@ -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);
 }
+
+#define GRUB_SERIAL_PORT_NUM 4
+static char com_names[GRUB_SERIAL_PORT_NUM][20];
+static struct grub_serial_port com_ports[GRUB_SERIAL_PORT_NUM];
+static int ports = 0;
+
+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 */
+  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
+       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
+      ((class >> 8) & 0xff) != 0x02)
+	  return 0;
+
+  if (!(bar & 1)) /* Not I/O */
+	  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;
+  err = grub_serial_config_defaults (&com_ports[i]);
+  if (err) {
+	  grub_print_error();
+	  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
 
+#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
+#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
+
 #ifndef ASM_FILE
 
 enum

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

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

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

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

[Peter Zijlstra]

On Wed, Aug 24, 2022 at 06:36:27PM -0500, Glenn Washburn wrote:
> 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.

Oh boy... you don't happen to have a convenient GRUB_HAZ_PCI define
squirreld away somewhere eh?


Does this look like something that could work?

---
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 5212dfab1369..ba52a675da84 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -224,12 +224,14 @@ kernel = {
 
   i386_efi = kern/i386/efi/init.c;
   i386_efi = bus/pci.c;
+  i386_efi_cppflags = '-DGRUB_HAS_PCI'
 
   x86_64 = kern/x86_64/dl.c;
   x86_64_xen = kern/x86_64/dl.c;
   x86_64_efi = kern/x86_64/efi/callwrap.S;
   x86_64_efi = kern/i386/efi/init.c;
   x86_64_efi = bus/pci.c;
+  x86_64_efi_cppflags = '-DGRUB_HAS_PCI'
 
   xen = kern/i386/tsc.c;
   xen = kern/i386/xen/tsc.c;
@@ -271,6 +273,7 @@ kernel = {
   i386_pc = term/i386/pc/console.c;
 
   i386_qemu = bus/pci.c;
+  i386_qemu_cppflags = '-DGRUB_HAS_PCI'
   i386_qemu = kern/vga_init.c;
   i386_qemu = kern/i386/qemu/mmap.c;
 
@@ -303,6 +306,7 @@ kernel = {
   mips_loongson = bus/bonito.c;
   mips_loongson = bus/cs5536.c;
   mips_loongson = bus/pci.c;
+  mips_loongson_cppflags = '-DGRUB_HAS_PCI'
   mips_loongson = kern/mips/loongson/init.c;
   mips_loongson = term/at_keyboard.c;
   mips_loongson = term/ps2.c;
@@ -2047,6 +2051,8 @@ module = {
   ieee1275 = term/ieee1275/serial.c;
   mips_arc = term/arc/serial.c;
   efi = term/efi/serial.c;
+  x86 = term/pci/serial.c;
+  mips_loongson = term/pci/serial.c;
 
   enable = terminfomodule;
   enable = ieee1275;

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

[Peter Zijlstra]

On Wed, Aug 24, 2022 at 06:36:27PM -0500, Glenn Washburn wrote:
> You will need to run the bootstrap script in the root of the repo to
> rebuild the build scripts before running configure and make.

FWIW bootstrap scribbles INSTALL

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

[Gerd Hoffmann]

  Hi,

> GRUB_SERIAL_COMMAND="serial --port=0x40a0 --speed=115200"

> +  grub_snprintf (com_names[i], sizeof (com_names[i]), "pci%d", i);

So this patch goes register the device(s) as 'pciN', so you don't have
to figure the ioport but can just use 'serial pci0' ?

take care,
  Gerd

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

[Peter Zijlstra]

How's this then? seems to build. If this looks allright, I'll post it as
v2 I suppose.

---
diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 5212dfab1369..c0683da2353b 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -224,12 +224,14 @@ kernel = {
 
   i386_efi = kern/i386/efi/init.c;
   i386_efi = bus/pci.c;
+  i386_efi_cppflags = '-DGRUB_HAS_PCI';
 
   x86_64 = kern/x86_64/dl.c;
   x86_64_xen = kern/x86_64/dl.c;
   x86_64_efi = kern/x86_64/efi/callwrap.S;
   x86_64_efi = kern/i386/efi/init.c;
   x86_64_efi = bus/pci.c;
+  x86_64_efi_cppflags = '-DGRUB_HAS_PCI';
 
   xen = kern/i386/tsc.c;
   xen = kern/i386/xen/tsc.c;
@@ -271,6 +273,7 @@ kernel = {
   i386_pc = term/i386/pc/console.c;
 
   i386_qemu = bus/pci.c;
+  i386_qemu_cppflags = '-DGRUB_HAS_PCI';
   i386_qemu = kern/vga_init.c;
   i386_qemu = kern/i386/qemu/mmap.c;
 
@@ -303,6 +306,7 @@ kernel = {
   mips_loongson = bus/bonito.c;
   mips_loongson = bus/cs5536.c;
   mips_loongson = bus/pci.c;
+  mips_loongson_cppflags = '-DGRUB_HAS_PCI';
   mips_loongson = kern/mips/loongson/init.c;
   mips_loongson = term/at_keyboard.c;
   mips_loongson = term/ps2.c;
@@ -2047,6 +2051,8 @@ module = {
   ieee1275 = term/ieee1275/serial.c;
   mips_arc = term/arc/serial.c;
   efi = term/efi/serial.c;
+  x86 = term/pci/serial.c;
+  mips_loongson = term/pci/serial.c;
 
   enable = terminfomodule;
   enable = ieee1275;
diff --git a/grub-core/term/pci/serial.c b/grub-core/term/pci/serial.c
new file mode 100644
index 000000000000..581d0760f9f4
--- /dev/null
+++ b/grub-core/term/pci/serial.c
@@ -0,0 +1,83 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2000,2001,2002,2003,2004,2005,2007,2008,2009,2010  Free Software Foundation, Inc.
+ *
+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <grub/serial.h>
+#include <grub/term.h>
+#include <grub/types.h>
+#include <grub/pci.h>
+#include <grub/mm.h>
+#include <grub/misc.h>
+
+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;
+  struct grub_serial_port *port;
+  grub_uint32_t class, bar;
+  grub_uint16_t cmdreg;
+  int *nr = data;
+  int err;
+
+  (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);
+
+  /* 16550 compattible MODEM or SERIAL */
+  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
+       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
+      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
+    return 0;
+
+  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
+    return 0;
+
+  port = grub_zalloc (sizeof (*port));
+  if (!port)
+    return 0;
+
+  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
+
+  port->name = grub_xasprintf ("pci%d", (*nr)++);
+  port->driver = &grub_ns8250_driver;
+  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
+  err = grub_serial_config_defaults (port);
+  if (err)
+    {
+      grub_print_error ();
+      return 0;
+    }
+
+  grub_serial_register (port);
+
+  return 0;
+}
+
+void
+grub_pciserial_init (void)
+{
+  int nr = 0;
+
+  grub_pci_iterate (find_pciserial, &nr);
+}
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 842322b1c607..09c9e1cd1fdc 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -448,6 +448,9 @@ GRUB_MOD_INIT(serial)
 #ifdef GRUB_MACHINE_ARC
   grub_arcserial_init ();
 #endif
+#ifdef GRUB_HAS_PCI
+  grub_pciserial_init ();
+#endif
 }
 
 GRUB_MOD_FINI(serial)
diff --git a/include/grub/pci.h b/include/grub/pci.h
index 262c89b748bb..b228bb1b71fd 100644
--- a/include/grub/pci.h
+++ b/include/grub/pci.h
@@ -80,8 +80,13 @@
 
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
+
 #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
 #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
+#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
+#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
+
+#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
 
 #ifndef ASM_FILE
 
diff --git a/include/grub/serial.h b/include/grub/serial.h
index 67379de45dd1..c57aed19ff2b 100644
--- a/include/grub/serial.h
+++ b/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);

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

Like, so eh? :-)

quilt new grub-rename-pci-class.patch;
git grep -l -e GRUB_PCI_CLASS_SUBCLASS_USB -e GRUB_PCI_CLASS_SUBCLASS_VGA | while read file;
do
	quilt add $file;
	sed -i -e 's/GRUB_PCI_CLASS_SUBCLASS_USB/GRUB_PCI_CLASS_SERIAL_USB/g' -e 's/GRUB_PCI_CLASS_SUBCLASS_VGA/GRUB_PCI_CLASS_DISPLAY_VGA/g' $file;
done

---
Index: grub/grub-core/kern/i386/qemu/init.c
===================================================================
--- grub.orig/grub-core/kern/i386/qemu/init.c
+++ grub/grub-core/kern/i386/qemu/init.c
@@ -168,11 +168,11 @@ enable_cards (grub_pci_device_t dev,
 
   class = (grub_pci_read (addr) >> 16) & 0xffff;
 
-  if (class == GRUB_PCI_CLASS_SUBCLASS_VGA)
+  if (class == GRUB_PCI_CLASS_DISPLAY_VGA)
     cmd |= GRUB_PCI_COMMAND_IO_ENABLED
       | GRUB_PCI_COMMAND_MEM_ENABLED;
 
-  if (class == GRUB_PCI_CLASS_SUBCLASS_USB)
+  if (class == GRUB_PCI_CLASS_SERIAL_USB)
     return 0;
 
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
Index: grub/grub-core/video/efi_uga.c
===================================================================
--- grub.orig/grub-core/video/efi_uga.c
+++ grub/grub-core/video/efi_uga.c
@@ -100,7 +100,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   subclass = (grub_pci_read (addr) >> 16) & 0xffff;
 
-  if (subclass != GRUB_PCI_CLASS_SUBCLASS_VGA)
+  if (subclass != GRUB_PCI_CLASS_DISPLAY_VGA)
     return 0;
 
   /* Enable MEM address spaces */
Index: grub/grub-core/video/radeon_fuloong2e.c
===================================================================
--- grub.orig/grub-core/video/radeon_fuloong2e.c
+++ grub/grub-core/video/radeon_fuloong2e.c
@@ -72,7 +72,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != 0x515a1002)
     return 0;
 
Index: grub/grub-core/video/radeon_yeeloong3a.c
===================================================================
--- grub.orig/grub-core/video/radeon_yeeloong3a.c
+++ grub/grub-core/video/radeon_yeeloong3a.c
@@ -71,7 +71,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != 0x96151002)
     return 0;
 
Index: grub/grub-core/video/sis315pro.c
===================================================================
--- grub.orig/grub-core/video/sis315pro.c
+++ grub/grub-core/video/sis315pro.c
@@ -100,7 +100,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != GRUB_SIS315PRO_PCIID)
     return 0;
 
Index: grub/grub-core/video/sm712.c
===================================================================
--- grub.orig/grub-core/video/sm712.c
+++ grub/grub-core/video/sm712.c
@@ -372,7 +372,7 @@ find_card (grub_pci_device_t dev, grub_p
   addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
   class = grub_pci_read (addr);
 
-  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
+  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
       || pciid != GRUB_SM712_PCIID)
     return 0;
 
Index: grub/include/grub/pci.h
===================================================================
--- grub.orig/include/grub/pci.h
+++ grub/include/grub/pci.h
@@ -81,8 +81,8 @@
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
 #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
 
-#define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
-#define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
+#define  GRUB_PCI_CLASS_DISPLAY_VGA           0x0300
+#define  GRUB_PCI_CLASS_SERIAL_USB            0x0c03
 #define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
 #define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
 

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

[Glenn Washburn]

On Thu, 25 Aug 2022 10:17:44 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

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

Yep, exactly what I was thinking. Thanks.

Glenn

> 
> quilt new grub-rename-pci-class.patch;
> git grep -l -e GRUB_PCI_CLASS_SUBCLASS_USB -e GRUB_PCI_CLASS_SUBCLASS_VGA | while read file;
> do
> 	quilt add $file;
> 	sed -i -e 's/GRUB_PCI_CLASS_SUBCLASS_USB/GRUB_PCI_CLASS_SERIAL_USB/g' -e 's/GRUB_PCI_CLASS_SUBCLASS_VGA/GRUB_PCI_CLASS_DISPLAY_VGA/g' $file;
> done
> 
> ---
> Index: grub/grub-core/kern/i386/qemu/init.c
> ===================================================================
> --- grub.orig/grub-core/kern/i386/qemu/init.c
> +++ grub/grub-core/kern/i386/qemu/init.c
> @@ -168,11 +168,11 @@ enable_cards (grub_pci_device_t dev,
>  
>    class = (grub_pci_read (addr) >> 16) & 0xffff;
>  
> -  if (class == GRUB_PCI_CLASS_SUBCLASS_VGA)
> +  if (class == GRUB_PCI_CLASS_DISPLAY_VGA)
>      cmd |= GRUB_PCI_COMMAND_IO_ENABLED
>        | GRUB_PCI_COMMAND_MEM_ENABLED;
>  
> -  if (class == GRUB_PCI_CLASS_SUBCLASS_USB)
> +  if (class == GRUB_PCI_CLASS_SERIAL_USB)
>      return 0;
>  
>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_COMMAND);
> Index: grub/grub-core/video/efi_uga.c
> ===================================================================
> --- grub.orig/grub-core/video/efi_uga.c
> +++ grub/grub-core/video/efi_uga.c
> @@ -100,7 +100,7 @@ find_card (grub_pci_device_t dev, grub_p
>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
>    subclass = (grub_pci_read (addr) >> 16) & 0xffff;
>  
> -  if (subclass != GRUB_PCI_CLASS_SUBCLASS_VGA)
> +  if (subclass != GRUB_PCI_CLASS_DISPLAY_VGA)
>      return 0;
>  
>    /* Enable MEM address spaces */
> Index: grub/grub-core/video/radeon_fuloong2e.c
> ===================================================================
> --- grub.orig/grub-core/video/radeon_fuloong2e.c
> +++ grub/grub-core/video/radeon_fuloong2e.c
> @@ -72,7 +72,7 @@ find_card (grub_pci_device_t dev, grub_p
>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
>    class = grub_pci_read (addr);
>  
> -  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
> +  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
>        || pciid != 0x515a1002)
>      return 0;
>  
> Index: grub/grub-core/video/radeon_yeeloong3a.c
> ===================================================================
> --- grub.orig/grub-core/video/radeon_yeeloong3a.c
> +++ grub/grub-core/video/radeon_yeeloong3a.c
> @@ -71,7 +71,7 @@ find_card (grub_pci_device_t dev, grub_p
>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
>    class = grub_pci_read (addr);
>  
> -  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
> +  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
>        || pciid != 0x96151002)
>      return 0;
>  
> Index: grub/grub-core/video/sis315pro.c
> ===================================================================
> --- grub.orig/grub-core/video/sis315pro.c
> +++ grub/grub-core/video/sis315pro.c
> @@ -100,7 +100,7 @@ find_card (grub_pci_device_t dev, grub_p
>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
>    class = grub_pci_read (addr);
>  
> -  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
> +  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
>        || pciid != GRUB_SIS315PRO_PCIID)
>      return 0;
>  
> Index: grub/grub-core/video/sm712.c
> ===================================================================
> --- grub.orig/grub-core/video/sm712.c
> +++ grub/grub-core/video/sm712.c
> @@ -372,7 +372,7 @@ find_card (grub_pci_device_t dev, grub_p
>    addr = grub_pci_make_address (dev, GRUB_PCI_REG_CLASS);
>    class = grub_pci_read (addr);
>  
> -  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_SUBCLASS_VGA
> +  if (((class >> 16) & 0xffff) != GRUB_PCI_CLASS_DISPLAY_VGA
>        || pciid != GRUB_SM712_PCIID)
>      return 0;
>  
> Index: grub/include/grub/pci.h
> ===================================================================
> --- grub.orig/include/grub/pci.h
> +++ grub/include/grub/pci.h
> @@ -81,8 +81,8 @@
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
>  
> -#define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
> -#define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> +#define  GRUB_PCI_CLASS_DISPLAY_VGA           0x0300
> +#define  GRUB_PCI_CLASS_SERIAL_USB            0x0c03
>  #define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
>  #define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
>  

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

[Glenn Washburn]

On Thu, 25 Aug 2022 09:57:32 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Aug 24, 2022 at 06:36:27PM -0500, Glenn Washburn wrote:
> > You will need to run the bootstrap script in the root of the repo to
> > rebuild the build scripts before running configure and make.
> 
> FWIW bootstrap scribbles INSTALL

Its been a while since I've looked at this. But I believe that INSTALL
is copied to INSTALL.grub. Is INSTALL getting overwritten with a
default INSTALL not related to GRUB? I'm unsure of what you're really
wanting to convey here. Something actionable?

Glenn

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

[Glenn Washburn]

On Thu, 25 Aug 2022 10:10:45 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> 
> 
> How's this then? seems to build. If this looks allright, I'll post it as
> v2 I suppose.

Yes, this is what I was thinking, with the nice addition of
GRUB_HAS_PCI, which I wasn't considering.

> 
> ---
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 5212dfab1369..c0683da2353b 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -224,12 +224,14 @@ kernel = {
>  
>    i386_efi = kern/i386/efi/init.c;
>    i386_efi = bus/pci.c;
> +  i386_efi_cppflags = '-DGRUB_HAS_PCI';

I imagine this should be enabled for all i386 and x86_64 targets, not
just the few that you have here. For instance, what about i386-pc,
which is the 32-bit BIOS target?

>  
>    x86_64 = kern/x86_64/dl.c;
>    x86_64_xen = kern/x86_64/dl.c;
>    x86_64_efi = kern/x86_64/efi/callwrap.S;
>    x86_64_efi = kern/i386/efi/init.c;
>    x86_64_efi = bus/pci.c;
> +  x86_64_efi_cppflags = '-DGRUB_HAS_PCI';
>  
>    xen = kern/i386/tsc.c;
>    xen = kern/i386/xen/tsc.c;
> @@ -271,6 +273,7 @@ kernel = {
>    i386_pc = term/i386/pc/console.c;
>  
>    i386_qemu = bus/pci.c;
> +  i386_qemu_cppflags = '-DGRUB_HAS_PCI';
>    i386_qemu = kern/vga_init.c;
>    i386_qemu = kern/i386/qemu/mmap.c;
>  
> @@ -303,6 +306,7 @@ kernel = {
>    mips_loongson = bus/bonito.c;
>    mips_loongson = bus/cs5536.c;
>    mips_loongson = bus/pci.c;
> +  mips_loongson_cppflags = '-DGRUB_HAS_PCI';
>    mips_loongson = kern/mips/loongson/init.c;
>    mips_loongson = term/at_keyboard.c;
>    mips_loongson = term/ps2.c;
> @@ -2047,6 +2051,8 @@ module = {
>    ieee1275 = term/ieee1275/serial.c;
>    mips_arc = term/arc/serial.c;
>    efi = term/efi/serial.c;
> +  x86 = term/pci/serial.c;
> +  mips_loongson = term/pci/serial.c;
>  
>    enable = terminfomodule;
>    enable = ieee1275;
> diff --git a/grub-core/term/pci/serial.c b/grub-core/term/pci/serial.c
> new file mode 100644
> index 000000000000..581d0760f9f4
> --- /dev/null
> +++ b/grub-core/term/pci/serial.c
> @@ -0,0 +1,83 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2000,2001,2002,2003,2004,2005,2007,2008,2009,2010  Free Software Foundation, Inc.

s/2000,2001,2002,2003,2004,2005,2007,2008,2009,2010/2022/

> + *
> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <grub/serial.h>
> +#include <grub/term.h>
> +#include <grub/types.h>
> +#include <grub/pci.h>
> +#include <grub/mm.h>
> +#include <grub/misc.h>
> +
> +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;
> +  struct grub_serial_port *port;
> +  grub_uint32_t class, bar;
> +  grub_uint16_t cmdreg;
> +  int *nr = data;

I'd prefer a more descriptive name, like port_num.

> +  int err;

s/int/grub_err_t/

> +
> +  (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);
> +
> +  /* 16550 compattible MODEM or SERIAL */
> +  if (((class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_MODEM &&
> +       (class >> 16) != GRUB_PCI_CLASS_COMMUNICATION_SERIAL) ||
> +      ((class >> 8) & 0xff) != GRUB_PCI_SERIAL_16550_COMPATIBLE)
> +    return 0;
> +
> +  if ((bar & GRUB_PCI_ADDR_SPACE_MASK) != GRUB_PCI_ADDR_SPACE_IO)
> +    return 0;
> +
> +  port = grub_zalloc (sizeof (*port));
> +  if (!port)

Daniel prefers: port != NULL

> +    return 0;
> +
> +  grub_pci_write (cmd_addr, cmdreg | GRUB_PCI_COMMAND_IO_ENABLED);
> +
> +  port->name = grub_xasprintf ("pci%d", (*nr)++);

Incrementing nr should happen just before the return on the success
path.

I believe there will be a memory leak when removing the serial module.
And I'm noticing that there appears to be a memory leak in the efi
serial code also. So maybe this is acceptable.

There should probably be code like:

  if (port->name == NULL)
    goto error;

> +  port->driver = &grub_ns8250_driver;
> +  port->port = bar & GRUB_PCI_ADDR_IO_MASK;
> +  err = grub_serial_config_defaults (port);
> +  if (err)

  if (err != GRUB_ERR_NONE)

> +    {
> +      grub_print_error ();
> +      return 0;

  goto error;

> +    }
> +
> +  grub_serial_register (port);

  err = grub_serial_register (port);
  if (err != GRUB_ERR_NONE)
    goto error;

  (*port_num)++;

> +
> +  return 0;

 error:
   grub_free (port->name);
   grub_free (port);
   return 0;

> +}
> +
> +void
> +grub_pciserial_init (void)
> +{
> +  int nr = 0;

Let's rename this also.

> +
> +  grub_pci_iterate (find_pciserial, &nr);
> +}
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index 842322b1c607..09c9e1cd1fdc 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -448,6 +448,9 @@ GRUB_MOD_INIT(serial)
>  #ifdef GRUB_MACHINE_ARC
>    grub_arcserial_init ();
>  #endif
> +#ifdef GRUB_HAS_PCI
> +  grub_pciserial_init ();
> +#endif
>  }
>  
>  GRUB_MOD_FINI(serial)
> diff --git a/include/grub/pci.h b/include/grub/pci.h
> index 262c89b748bb..b228bb1b71fd 100644
> --- a/include/grub/pci.h
> +++ b/include/grub/pci.h
> @@ -80,8 +80,13 @@
>  
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_SHIFT 9
>  #define  GRUB_PCI_STATUS_DEVSEL_TIMING_MASK 0x0600
> +
>  #define  GRUB_PCI_CLASS_SUBCLASS_VGA  0x0300
>  #define  GRUB_PCI_CLASS_SUBCLASS_USB  0x0c03
> +#define  GRUB_PCI_CLASS_COMMUNICATION_SERIAL  0x0700
> +#define  GRUB_PCI_CLASS_COMMUNICATION_MODEM   0x0703
> +
> +#define  GRUB_PCI_SERIAL_16550_COMPATIBLE 0x02
>  
>  #ifndef ASM_FILE
>  
> diff --git a/include/grub/serial.h b/include/grub/serial.h
> index 67379de45dd1..c57aed19ff2b 100644
> --- a/include/grub/serial.h
> +++ b/include/grub/serial.h
> @@ -193,6 +193,8 @@ const char *
>  grub_arcserial_add_port (const char *path);
>  #endif
>  
> +void grub_pciserial_init (void);

I think this should be wrapped in "#ifdef GRUB_HAS_PCI" for clarity of
use.

Glenn

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

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

[Peter Zijlstra]

On Thu, Aug 25, 2022 at 10:51:29AM -0500, Glenn Washburn wrote:
> On Thu, 25 Aug 2022 09:57:32 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Aug 24, 2022 at 06:36:27PM -0500, Glenn Washburn wrote:
> > > You will need to run the bootstrap script in the root of the repo to
> > > rebuild the build scripts before running configure and make.
> > 
> > FWIW bootstrap scribbles INSTALL
> 
> Its been a while since I've looked at this. But I believe that INSTALL
> is copied to INSTALL.grub. Is INSTALL getting overwritten with a
> default INSTALL not related to GRUB? I'm unsure of what you're really
> wanting to convey here. Something actionable?

When you do ./bootstrap; git diff returns a fairly sizable diff on
INSTALL, which is a bit weird and tends to sneak into patches (had to
kill it twice etc..).

So either add INSTALL to .gitignore, or fix it so that INSTALL doesn't
get scribbled would be my suggestion.

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

[Peter Zijlstra]

On Thu, Aug 25, 2022 at 02:23:34PM -0500, Glenn Washburn wrote:

> > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > index 5212dfab1369..c0683da2353b 100644
> > --- a/grub-core/Makefile.core.def
> > +++ b/grub-core/Makefile.core.def
> > @@ -224,12 +224,14 @@ kernel = {
> >  
> >    i386_efi = kern/i386/efi/init.c;
> >    i386_efi = bus/pci.c;
> > +  i386_efi_cppflags = '-DGRUB_HAS_PCI';
> 
> I imagine this should be enabled for all i386 and x86_64 targets, not
> just the few that you have here. For instance, what about i386-pc,
> which is the 32-bit BIOS target?

So what I did was put that _cppflags thing right next to all the
'bus/pci.c' instances I found, with exception of the pci module.

Where do i386-pc and friends get it from? IOW, what obvious place did I
miss?

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

[Glenn Washburn]

On Thu, 25 Aug 2022 21:33:31 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Aug 25, 2022 at 10:51:29AM -0500, Glenn Washburn wrote:
> > On Thu, 25 Aug 2022 09:57:32 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Wed, Aug 24, 2022 at 06:36:27PM -0500, Glenn Washburn wrote:
> > > > You will need to run the bootstrap script in the root of the repo to
> > > > rebuild the build scripts before running configure and make.
> > > 
> > > FWIW bootstrap scribbles INSTALL
> > 
> > Its been a while since I've looked at this. But I believe that INSTALL
> > is copied to INSTALL.grub. Is INSTALL getting overwritten with a
> > default INSTALL not related to GRUB? I'm unsure of what you're really
> > wanting to convey here. Something actionable?
> 
> When you do ./bootstrap; git diff returns a fairly sizable diff on
> INSTALL, which is a bit weird and tends to sneak into patches (had to
> kill it twice etc..).
> 
> So either add INSTALL to .gitignore, or fix it so that INSTALL doesn't
> get scribbled would be my suggestion.

Strange, I don't see that behavior. Maybe its your autotools version.

Glenn

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

[Glenn Washburn]

On Thu, 25 Aug 2022 21:59:01 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Thu, Aug 25, 2022 at 02:23:34PM -0500, Glenn Washburn wrote:
> 
> > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> > > index 5212dfab1369..c0683da2353b 100644
> > > --- a/grub-core/Makefile.core.def
> > > +++ b/grub-core/Makefile.core.def
> > > @@ -224,12 +224,14 @@ kernel = {
> > >  
> > >    i386_efi = kern/i386/efi/init.c;
> > >    i386_efi = bus/pci.c;
> > > +  i386_efi_cppflags = '-DGRUB_HAS_PCI';
> > 
> > I imagine this should be enabled for all i386 and x86_64 targets, not
> > just the few that you have here. For instance, what about i386-pc,
> > which is the 32-bit BIOS target?
> 
> So what I did was put that _cppflags thing right next to all the
> 'bus/pci.c' instances I found, with exception of the pci module.
> 
> Where do i386-pc and friends get it from? IOW, what obvious place did I
> miss?

The main missed place is in the definition of the serial module in
Makefile.core.def. That module will not be built with -DGRUB_HAS_PCI
even on tagets, like i386-efi, in which you added -DGRUB_HAS_PCI in the
kernel. Because the CPPFLAGS for the kernel don't get used for the
modules.

Looking at this more and its worse than I thought. What we want is
for all pci enabled builds to get -DGRUB_HAS_PCI. You just added it for
C/C++ files used when building the kernel, but it should happen for all
C/C++ files on targets that GRUB supports PCI. It doesn't appear there
is a good way to do this right now.

So I've created a patch that I think is the RightWay(TM) to do it (see
patch titled "[PATCH] configure: Add -DGRUB_HAS_PCI when compiling
C/C++ files on targets that support PCI"). Using this patch you should
be able to use GRUB_HAS_PCI in the C files. The next patch version
should not include -DGRUB_HAS_PCI in the Makefile.core.def.

Also, the added lines:

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

Should be instead be one line:

  pci = term/pci/serial.c;

Let me know if something doesn't work.

Glenn