T8120: Add support AMA console for ARM devices #4915
Conversation
|
👍 |
There was a problem hiding this comment.
Pull request overview
This PR adds support for ARM Advanced Microcontroller Bus Architecture (AMBA) PL011 serial console devices (ttyAMA) to VyOS, extending the existing console options beyond KVM (graphical) and standard serial (ttyS) consoles.
Key Changes:
- Adds 'A' (ARM serial) as a console type option during image installation
- Extends GRUB configuration to support ttyAMA console devices
- Updates system console XML schema to validate ttyAMA device names
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/op_mode/image_installer.py | Adds 'A' option for ARM serial console selection and ttyAMA0 detection in console_hint() function |
| interface-definitions/system_console.xml.in | Adds ttyAMA validation pattern and documentation for ARM serial ports |
| data/templates/grub/grub_vyos_version.j2 | Adds conditional logic for ttyAMA console options in kernel command line |
| data/templates/grub/grub_options.j2 | Adds ttyAMA menu entry in GRUB boot options |
| data/templates/grub/grub_compat.j2 | Adds console name and options macros for ARM serial console compatibility |
| data/templates/grub/grub_common.j2 | Implements serial_pl011 initialization for ARM UART devices |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sever-sever
force-pushed
the
T8120
branch
2 times, most recently
from
64f33f7 to
27c951b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
data/templates/grub/grub_compat.j2:47
- The serial port initialization section (lines 39-47) does not handle ttyAMA console type. While ttyAMA is handled in the console_name and console_opts macros, this section that sets up the GRUB serial console is missing a case for ttyAMA. This should include a check for
console_type == 'ttyAMA'similar to the existing ttyS handling to ensure proper serial port initialization.
{% if console_type == 'ttyS' %}
{% if console_num == '0' %}
serial --unit=0 --speed={{ console_speed }}
{% else %}
serial --unit={{ console_num }} --speed=115200
{% endif %}
{% else %}
serial --unit=0 --speed={{ console_speed }}
{% endif %}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/system/grub_update.py:89
- The
archtemplate variable is added tovarsbut not passed to the grub_options template. Change the empty dict{}tovarsso the architecture information is available in the template:render(grub_cfg_options, grub.TMPL_GRUB_OPTS, vars)
render(grub_cfg_options, grub.TMPL_GRUB_OPTS, {})
interface-definitions/system_console.xml.in:15
- The completion script should be updated to include ttyAMA devices:
<script>ls -1 /dev | grep -e ttyS -e ttyAMA -e hvc</script>
<script>ls -1 /dev | grep -e ttyS -e hvc</script>
interface-definitions/system_console.xml.in:21
- Add a valueHelp entry for ttyAMA devices to provide documentation for users. Insert after line 21:
<valueHelp><format>ttyAMAN</format><description>ARM Advanced AMBA serial port</description></valueHelp>
<valueHelp>
<format>ttySN</format>
<description>TTY device name, regular serial port</description>
</valueHelp>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
data/templates/grub/grub_options.j2:49
- The GRUB
menuentry "Enter console number"readsconsole_numdirectly from the user and then passes it tosetup_serialand later into the kernel boot options without validation. By entering a value containing shell metacharacters or additional arguments (e.g.0; linux /boot/vmlinuz init=/bin/sh; bootor0 init=/bin/sh), an attacker with boot-console access can inject arbitrary GRUB commands or kernel parameters and boot into a root shell bypassing normal authentication. Restrictconsole_numto a strict numeric pattern (digits only) and/or sanitize it before using it in GRUB commands and when constructingconsole_opts.
read console_num
export console_num
setup_serial
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
zdc
left a comment
zdc
left a comment
There was a problem hiding this comment.
We should find all places where we have a call to set_console_type(), and add S/AMA detection there.
They seem to be:
vyos-1x/src/op_mode/image_installer.py
Lines 745 to 756 in 0c373c7
vyos-1x/src/op_mode/image_installer.py
Line 985 in 0c373c7
Also add AMA here: https://github.com/vyos/vyos-1x/blob/current/src/op_mode/image_manager.py#L36
data/templates/grub/grub_options.j2
Outdated
| {% if arch == 'aarch64' %} | ||
| menuentry "ttyAMA (serial)" { | ||
| set console_type="ttyAMA" | ||
| export console_type | ||
| setup_serial | ||
| configfile ${prefix}/grub.cfg.d/*vyos-menu*.cfg | ||
| } | ||
| {% else %} | ||
| menuentry "ttyS (serial)" { | ||
| set console_type="ttyS" | ||
| export console_type | ||
| setup_serial | ||
| configfile ${prefix}/grub.cfg.d/*vyos-menu*.cfg | ||
| } | ||
| {% endif %} |
There was a problem hiding this comment.
Do we need to branch the whole section? The difference is only in two lines.
And, actually, I want to offer considering renaming options to simply Graphical and Serial. Then, there will be no need to add any logic here at all. Instead, I would recommend considering using https://www.gnu.org/software/grub/manual/grub/html_node/grub_005fcpu.html to find a proper console name.
There was a problem hiding this comment.
Done, re-check please
sever-sever
force-pushed
the
T8120
branch
2 times, most recently
from
9d77bbb to
3254cb3
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sever-sever
force-pushed
the
T8120
branch
5 times, most recently
from
0afb8e4 to
40028c2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
data/templates/grub/grub_compat.j2:47
- The serial console setup only checks for 'ttyS' but should also handle 'ttyAMA' to properly configure serial console parameters for ARM devices. The condition should be
{% if console_type == 'ttyS' or console_type == 'ttyAMA' %}to match the pattern used in other updated templates.
{% if console_type == 'ttyS' %}
{% if console_num == '0' %}
serial --unit=0 --speed={{ console_speed }}
{% else %}
serial --unit={{ console_num }} --speed=115200
{% endif %}
{% else %}
serial --unit=0 --speed={{ console_speed }}
{% endif %}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
c-po
left a comment
c-po
left a comment
There was a problem hiding this comment.
All requested changes and CoPilot findings adressed.
Not manually tested but implementarion looks good to go!
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| menuentry "{{ version_name }}" --id {{ version_uuid }} { | ||
| set boot_opts="{{ boot_opts_rendered }}" | ||
| if [ "${console_type}" == "ttyS" ]; then | ||
| if [ "${console_type}" = "ttyS" -o "${console_type}" = "ttyAMA" ]; then |
There was a problem hiding this comment.
Inconsistent operator usage in GRUB shell script. The code mixes '==' (line 11, 20, 23, 25) and '=' (line 14) for string comparisons. While both work in GRUB, consider using '==' consistently throughout the file for better code consistency. The existing code already uses '==' for similar string comparisons.
| if [ "${console_type}" = "ttyS" -o "${console_type}" = "ttyAMA" ]; then | |
| if [ "${console_type}" == "ttyS" -o "${console_type}" == "ttyAMA" ]; then |
| menuentry "ttyS (serial)" { | ||
| set console_type="ttyS" | ||
| if [ "${grub_cpu}" = "arm64" ]; then | ||
| set serial_console="ttyAMA" | ||
| else | ||
| set serial_console="ttyS" | ||
| fi | ||
| set console_type="$serial_console" |
There was a problem hiding this comment.
The menu entry label "ttyS (serial)" is misleading on ARM platforms where it actually sets the console to "ttyAMA". Consider updating the label to be more generic, such as "Serial console" or making it architecture-aware to improve user clarity.
|
CI integration 👍 passed! Details
|
3 participants
Change summary
Add support AMA console for ARM devices
Types of changes
Related Task(s)
Related PR(s)
How to test / Smoketest result
Checklist: