Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Fri, Jan 20, 2023 at 05:15:48PM +1100, Benjamin Herrenschmidt wrote:
> On Thu, 2023-01-19 at 18:31 +0100, Daniel Kiper wrote:
> >
> > Sadly your patch set broke various builds including x86-ieee1275 one. I had
> > to do some tweaks to make it work again. You can find most important ones
> > below. Please double check I have not messed up something.
>
> Oh, sorry about that. I didn't know there even was such a thing as x86-
> ieee1275. Is there an easy way to run / test all those builds ? A

Yeah, I was surprised too when I saw it first time... :-)

> script or service somewhere ?

./configure --target=i386 --with-platform=ieee1275 ...

Sadly we do not have any script yet. However, the INSTALL file has quite
good description how to setup a cross-platform build environment.

> > diff --git a/grub-core/term/ieee1275/serial.c 
> > b/grub-core/term/ieee1275/serial.c
> > index afbe8dcda..0e4cac4c4 100644
> > --- a/grub-core/term/ieee1275/serial.c
> > +++ b/grub-core/term/ieee1275/serial.c
> > @@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
> >  return NULL;
> >  
> >    FOR_SERIAL_PORTS (port)
> > -    if (port->ent == ent)
> > +    if (port->elem == ent)
> >    return port;
>
> Right.
>
> >    port = grub_zalloc (sizeof (*port));
> > @@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
> >    return port;
> >  }
> >  
> > -const struct grub_serial_port *
> > +struct grub_serial_port *
> >  grub_ofserial_add_port (const char *path)
> >  {
> >    struct ofserial_hash_ent *ent;
> > diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> > index 01533d969..d101bffb5 100644
> > --- a/grub-core/term/ns8250-spcr.c
> > +++ b/grub-core/term/ns8250-spcr.c
> > @@ -16,6 +16,8 @@
> >   *  along with GRUB.  If not, see .
> >   */
> >  
> > +#if !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
> > +
> >  #include 
> >  #include 
> >  #include 
> > @@ -82,3 +84,5 @@ grub_ns8250_spcr_init (void)
> >  return NULL;
> >  };
> >  }
>
> This is the rule for ACPI ? Or is something else problematic ? No
> particular objection here, just wondering.

IIRC this is due to lack of support for ACPI on some platforms.

> > +
> > +#endif
> > diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> > index 81abd570c..c65ffc63c 100644
> > --- a/grub-core/term/serial.c
> > +++ b/grub-core/term/serial.c
> > @@ -209,6 +209,8 @@ grub_serial_find (const char *name)
> >    if (port != NULL)
> >  return port;
> >  }
> > +
> > +#if (defined(__i386__) || defined(__x86_64__)) && 
> > !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
>
> Ok.
>
> >    if (grub_strcmp (name, "auto") == 0)
> >  {
> >    /* Look for an SPCR if any. If not, default to com0. */
> > @@ -220,6 +222,7 @@ grub_serial_find (const char *name)
> >    return port;
> >  }
> >  #endif
> > +#endif
> >  
> >  #ifdef GRUB_MACHINE_IEEE1275
> >    if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
> > diff --git a/include/grub/ieee1275/console.h 
> > b/include/grub/ieee1275/console.h
> > index bdd98fe06..a8094d381 100644
> > --- a/include/grub/ieee1275/console.h
> > +++ b/include/grub/ieee1275/console.h
> > @@ -28,7 +28,7 @@ void grub_console_init_lately (void);
> >  /* Finish the console system.  */
> >  void grub_console_fini (void);
> >  
> > -const char *
> > +struct grub_serial_port *
> >  grub_ofserial_add_port (const char *name);
> >
> Yeah, I didn't try building ieee1275, my fault. Sorry about that. Your
> changes seem fine to me.

No problem. Thanks a lot for checking this.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Benjamin Herrenschmidt]

On Thu, 2023-01-19 at 18:31 +0100, Daniel Kiper wrote:
> 
> Sadly your patch set broke various builds including x86-ieee1275 one. I had
> to do some tweaks to make it work again. You can find most important ones
> below. Please double check I have not messed up something.

Oh, sorry about that. I didn't know there even was such a thing as x86-
ieee1275. Is there an easy way to run / test all those builds ? A
script or service somewhere ?

> Daniel
> 
> diff --git a/grub-core/term/ieee1275/serial.c 
> b/grub-core/term/ieee1275/serial.c
> index afbe8dcda..0e4cac4c4 100644
> --- a/grub-core/term/ieee1275/serial.c
> +++ b/grub-core/term/ieee1275/serial.c
> @@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
>  return NULL;
>  
>    FOR_SERIAL_PORTS (port)
> -    if (port->ent == ent)
> +    if (port->elem == ent)
>    return port;

Right.

>    port = grub_zalloc (sizeof (*port));
> @@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
>    return port;
>  }
>  
> -const struct grub_serial_port *
> +struct grub_serial_port *
>  grub_ofserial_add_port (const char *path)
>  {
>    struct ofserial_hash_ent *ent;
> diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
> index 01533d969..d101bffb5 100644
> --- a/grub-core/term/ns8250-spcr.c
> +++ b/grub-core/term/ns8250-spcr.c
> @@ -16,6 +16,8 @@
>   *  along with GRUB.  If not, see .
>   */
>  
> +#if !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
> +
>  #include 
>  #include 
>  #include 
> @@ -82,3 +84,5 @@ grub_ns8250_spcr_init (void)
>  return NULL;
>  };
>  }

This is the rule for ACPI ? Or is something else problematic ? No
particular objection here, just wondering.

> +
> +#endif
> diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
> index 81abd570c..c65ffc63c 100644
> --- a/grub-core/term/serial.c
> +++ b/grub-core/term/serial.c
> @@ -209,6 +209,8 @@ grub_serial_find (const char *name)
>    if (port != NULL)
>  return port;
>  }
> +
> +#if (defined(__i386__) || defined(__x86_64__)) && 
> !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)

Ok.

>    if (grub_strcmp (name, "auto") == 0)
>  {
>    /* Look for an SPCR if any. If not, default to com0. */
> @@ -220,6 +222,7 @@ grub_serial_find (const char *name)
>    return port;
>  }
>  #endif
> +#endif
>  
>  #ifdef GRUB_MACHINE_IEEE1275
>    if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
> diff --git a/include/grub/ieee1275/console.h b/include/grub/ieee1275/console.h
> index bdd98fe06..a8094d381 100644
> --- a/include/grub/ieee1275/console.h
> +++ b/include/grub/ieee1275/console.h
> @@ -28,7 +28,7 @@ void grub_console_init_lately (void);
>  /* Finish the console system.  */
>  void grub_console_fini (void);
>  
> -const char *
> +struct grub_serial_port *
>  grub_ofserial_add_port (const char *name);
> 
Yeah, I didn't try building ieee1275, my fault. Sorry about that. Your
changes seem fine to me.

Cheers,
Ben.

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Thu, Jan 12, 2023 at 01:51:56PM +0100, Daniel Kiper wrote:
> On Fri, Dec 23, 2022 at 12:47:51PM +1100, Benjamin Herrenschmidt wrote:
> > This series adds support for MMIO serial ports and auto-configuration
> > via ACPI SPCR.
> >
> > This is necessary for the serial port to work on AWS EC2 "metal" x86
> > systems.
> >
> > An MMIO serial port can be specified explicitely using the
> > "mmio," prefix for the --port argument to the 'serial' command,
> > the register access size can also be specified via a suffix,
> > and when 'serial' is used without arguments, it will try to use
> > an ACPI SPCR entry (if any) to add the default port and configure
> > it appropriately (speed, parity etc...)
> >
> > This was tested using SPCR on an actual c5.metal instance, and using
> > explicit instanciation via serial -p mmio on a modified qemu
> > hacked to create MMIO PCI serial ports and to create a SPCR ACPI table.
> >
> > The insipiration was an original implementation by
> > Matthias Lange ! matthias.lange at kernkonzept.com
> > However, the code was rewritten pretty much from scratch.
>
> For all the patches Reviewed-by: Daniel Kiper ...

Sadly your patch set broke various builds including x86-ieee1275 one. I had
to do some tweaks to make it work again. You can find most important ones
below. Please double check I have not messed up something.

Daniel

diff --git a/grub-core/term/ieee1275/serial.c b/grub-core/term/ieee1275/serial.c
index afbe8dcda..0e4cac4c4 100644
--- a/grub-core/term/ieee1275/serial.c
+++ b/grub-core/term/ieee1275/serial.c
@@ -228,7 +228,7 @@ add_port (struct ofserial_hash_ent *ent)
 return NULL;
 
   FOR_SERIAL_PORTS (port)
-if (port->ent == ent)
+if (port->elem == ent)
   return port;
 
   port = grub_zalloc (sizeof (*port));
@@ -252,7 +252,7 @@ add_port (struct ofserial_hash_ent *ent)
   return port;
 }
 
-const struct grub_serial_port *
+struct grub_serial_port *
 grub_ofserial_add_port (const char *path)
 {
   struct ofserial_hash_ent *ent;
diff --git a/grub-core/term/ns8250-spcr.c b/grub-core/term/ns8250-spcr.c
index 01533d969..d101bffb5 100644
--- a/grub-core/term/ns8250-spcr.c
+++ b/grub-core/term/ns8250-spcr.c
@@ -16,6 +16,8 @@
  *  along with GRUB.  If not, see .
  */
 
+#if !defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
+
 #include 
 #include 
 #include 
@@ -82,3 +84,5 @@ grub_ns8250_spcr_init (void)
 return NULL;
 };
 }
+
+#endif
diff --git a/grub-core/term/serial.c b/grub-core/term/serial.c
index 81abd570c..c65ffc63c 100644
--- a/grub-core/term/serial.c
+++ b/grub-core/term/serial.c
@@ -209,6 +209,8 @@ grub_serial_find (const char *name)
   if (port != NULL)
 return port;
 }
+
+#if (defined(__i386__) || defined(__x86_64__)) && 
!defined(GRUB_MACHINE_IEEE1275) && !defined(GRUB_MACHINE_QEMU)
   if (grub_strcmp (name, "auto") == 0)
 {
   /* Look for an SPCR if any. If not, default to com0. */
@@ -220,6 +222,7 @@ grub_serial_find (const char *name)
   return port;
 }
 #endif
+#endif
 
 #ifdef GRUB_MACHINE_IEEE1275
   if (grub_strncmp (name, "ieee1275/", sizeof ("ieee1275/") - 1) == 0)
diff --git a/include/grub/ieee1275/console.h b/include/grub/ieee1275/console.h
index bdd98fe06..a8094d381 100644
--- a/include/grub/ieee1275/console.h
+++ b/include/grub/ieee1275/console.h
@@ -28,7 +28,7 @@ void grub_console_init_lately (void);
 /* Finish the console system.  */
 void grub_console_fini (void);
 
-const char *
+struct grub_serial_port *
 grub_ofserial_add_port (const char *name);
 
 #endif /* ! GRUB_CONSOLE_MACHINE_HEADER */

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 0/8] serial: Add MMIO & SPCR support

[Daniel Kiper]

On Fri, Dec 23, 2022 at 12:47:51PM +1100, Benjamin Herrenschmidt wrote:
> This series adds support for MMIO serial ports and auto-configuration
> via ACPI SPCR.
>
> This is necessary for the serial port to work on AWS EC2 "metal" x86
> systems.
>
> An MMIO serial port can be specified explicitely using the
> "mmio," prefix for the --port argument to the 'serial' command,
> the register access size can also be specified via a suffix,
> and when 'serial' is used without arguments, it will try to use
> an ACPI SPCR entry (if any) to add the default port and configure
> it appropriately (speed, parity etc...)
>
> This was tested using SPCR on an actual c5.metal instance, and using
> explicit instanciation via serial -p mmio on a modified qemu
> hacked to create MMIO PCI serial ports and to create a SPCR ACPI table.
>
> The insipiration was an original implementation by
> Matthias Lange ! matthias.lange at kernkonzept.com
> However, the code was rewritten pretty much from scratch.

For all the patches Reviewed-by: Daniel Kiper ...

Thank you for adding this feature!

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel