GbE-region MAC randomisation

[FckBigTch]

Follow-up to the PoC in #1195. This PR builds on that work and it is a feature to view and randomize the Ethernet MAC address stored in the GbE region of the flash.

Module

• Add nvmutil (from the Libreboot project).

Makefile

• Add ifdtool (from coreboot) as a build target in the Makefile, built directly from the board's coreboot checkout with Heads' existing cross toolchain to coreboot's own module configure step.

initrd/bin/gui-init.sh

• Add a "MAC address randomization" submenu in gui-init.sh, gated on boards config CONFIG_NVMUTIL=y and CONFIG_IFDTOOL=y.

GUI

New submenu under the main menu, shown only when both CONFIG_NVMUTIL and CONFIG_IFDTOOL are set to yes for the board:

• Show current Ethernet MAC address
• Randomize Ethernet MAC address (fully random)
• Randomize Ethernet MAC address using a valid OUI (locally-administered/universal OUI pattern, so the result still looks like a plausible vendor-assigned address)

Tested boards

• T480
• T480s
• x230
• not working on Novacustom v54 and v56, excluding GBE region PR Staging commits for v540tu/v560tu/ns50/n4x_adl: pr0 patch missing, ME HAP bit, s3, review + unify #1871

Known limitation / open question

Fbwhiptail doesn't support conditionally hiding a single menu entry, so gui-init.sh's show_options_menu is currently duplicated (one version with the MAC randomization entry, one without) selected based on whether the board has the feature enabled.
This works, but duplicating a whole menu function to add one conditional entry doesn't scale well if more optional, board-gated features are added later. An alternative I considered: always show the entry, and if CONFIG_NVMUTIL/CONFIG_IFDTOOL are no on that board, show a message like "This feature is not available on your board" instead of duplicating the menu. That avoids the duplication but surfaces a dead-end option to boards that can never use it. What do you think?

If there are anything changes you'd like me to make, just let me know.

[tlaurion]

Thanks for picking up on #1195 !

There is ways to make menu options dynamic, thinking of unlocking spi per boot per config settings as an example. Will look deeper in further code review

[tlaurion]

Confused on how ifdtool is built here vs #1195 (which was unclean still, required coreboot's built ifdtool that is needed to build coreboot (see modules/coreboot there) to add Makefile hacks to pack ifdtool as cbmem is packed to be used inside of Heads.

ifdtool needs to be built with musl-cross-make to depend on musl's libc; so unclear how building it with coreboot's buildstack produces an actual working ifdtool to be used inside of heads? You tested this pr @FckBigTch ?

[FckBigTch]

Confused on how ifdtool is built here vs #1195 (which was unclean still, required coreboot's built ifdtool that is needed to build coreboot (see modules/coreboot there) to add Makefile hacks to pack ifdtool as cbmem is packed to be used inside of Heads.

ifdtool needs to be built with musl-cross-make to depend on musl's libc; so unclear how building it with coreboot's buildstack produces an actual working ifdtool to be used inside of heads? You tested this pr @FckBigTch ?

Yes, I've tested several times, and flashed the resulting ROMs on t480s, t480 and x230. Ifdtool is usable inside of heads.
I just tried to do it like cbmem, superiotool or inteltool.
ifdtool is built from '$(build)/$(coreboot_dir)/util/ifdtool' using the board's own coreboot source tree, musl-cross-make and the board's DOTCONFIG, rather than duplicating the source tree.

Confirmed it's actually musl:

file build/x86/coreboot-25.09/util/ifdtool/ifdtool
build/x86/coreboot-25.09/util/ifdtool/ifdtool: ELF 64-bit LSB executable, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib/ld-musl-x86_64.so.1, with debug_info, not stripped

Interpreter is /lib/ld-musl-x86_64.so.1

[tlaurion]

Some notes from deeper review

• why we say it doesn't work with v540tu/v560tu? from a point of view of board config (flash command) v540tu/v560tu do the right thing here: they exclude for future internal flash the GBE region by specifying flash regions that are intended to be flashed only (desired, otherwise an internal upgrade would replace GBE with hardcoded mac) : export CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd"
  • Actually, by merging this PR, all boards that can have their mac address changed should have that flash command changed to exclude GBE from flashing from internal upgrades, otherwise replacing it in internal flashing of new rom ( right now : export CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal" so overwriting the whole flash )
  • What is missing in this PR is now a distinction for overriding temporarily (at runtime only) of the GBE region when flashing that section only with flashtool export from board config so changing GBE would only be done explicitly when the user wants to override its mac from config menu options, no? export CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd -i gbe"
• Still missing: dynamic menu option.
• Interesting gain here with ifdtool packed in, we could (other PR, not here) extract MRC cache and inject it in the rom so that memory training happens only if not matching (flashing rom with current commands overwirte the memory training cache, resulting in long boot times after flash for all DDR4, DDR5 taking up to near a minute before raminit is finished, which is the part drawing bootsplash on screen, leaving user in the dark up to that point.)

Will think about that a bit more. If you have ideas let me know @FckBigTch

[FckBigTch]

Some notes from deeper review

* why we say it doesn't work with v540tu/v560tu? from a point of view of board config (flash command) v540tu/v560tu do the right thing here: they exclude for future internal flash the GBE region by specifying flash regions that are intended to be flashed only (desired, otherwise an internal upgrade would replace GBE with hardcoded mac) :  `export CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd"`
  
  * Actually, by merging this PR, all boards that can have their mac address changed should have that flash command changed to exclude GBE from flashing from internal upgrades, otherwise replacing it in internal flashing of new rom ( right now : `export CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal"` so overwriting the whole flash )
  * What is missing in this PR is now a distinction for overriding temporarily (at runtime only) of the GBE region when flashing that section only with flashtool export from board config so changing GBE would only be done explicitly when the user wants to override its mac from config menu options, no? `export CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd -i gbe"`

* Still missing: dynamic menu option.

* Interesting gain here with ifdtool packed in, we could (other PR, not here) extract MRC cache and inject it in the rom so that memory training happens only if not matching (flashing rom with current commands overwirte the memory training cache, resulting in long boot times after flash for all DDR4, DDR5 taking up to near a minute before raminit is finished, which is the part drawing bootsplash on screen, leaving user in the dark up to that point.)

Will think about that a bit more. If you have ideas let me know @FckBigTch

The reason I thought the Novacustom models should be excluded is that the config explicitly states:
"NovaCustom V54 MTL (integrated graphics) board configuration
Note the FLASH_OPTIONS: '--ifd -i bios -i me -i fd'
This excludes gbe from internal flashing, otherwise mac address would revert to '88:88:88:88:87:88' see #1871 (comment)
Same options should be used when externally flashing the first time, otherwise Intel GbE region (Ethernet config blob) will be overwritten and MAC reverted to '88:88:88:88:87:88'.”

That means it's not possible to generate a random MAC address, right? But I understand that overwriting the GBE region should be avoided during an update.

I hadn't even thought of the idea with the dynamic menu. I'll follow your advice and try to implement it.

[tlaurion]

The reason I thought the Novacustom models should be excluded is that the config explicitly states:
"NovaCustom V54 MTL (integrated graphics) board configuration
Note the FLASH_OPTIONS: '--ifd -i bios -i me -i fd'
This excludes gbe from internal flashing, otherwise mac address would revert to '88:88:88:88:87:88' see #1871 (comment)
Same options should be used when externally flashing the first time, otherwise Intel GbE region (Ethernet config blob) will be overwritten and MAC reverted to '88:88:88:88:87:88'.”

That means it's not possible to generate a random MAC address, right? But I understand that overwriting the GBE region should be avoided during an update.

Misunderstanding. This means those boards pack a GBE (see coreboot configs) but don't ever flash it per initial flashing instructions or internal flashing. If someone misread initial flashing (which is done by OEM not end users in the case of these boards), then MAC address would become generic as per packed in rom GBE.

I hadn't even thought of the idea with the dynamic menu. I'll follow your advice and try to implement it.

[tlaurion]

The reason I thought the Novacustom models should be excluded is that the config explicitly states:
"NovaCustom V54 MTL (integrated graphics) board configuration
Note the FLASH_OPTIONS: '--ifd -i bios -i me -i fd'
This excludes gbe from internal flashing, otherwise mac address would revert to '88:88:88:88:87:88' see #1871 (comment)
Same options should be used when externally flashing the first time, otherwise Intel GbE region (Ethernet config blob) will be overwritten and MAC reverted to '88:88:88:88:87:88'.”

That means it's not possible to generate a random MAC address, right? But I understand that overwriting the GBE region should be avoided during an update.

Misunderstanding. This means those boards pack a GBE (see coreboot configs) but don't ever flash it per initial flashing instructions or internal flashing. If someone misread initial flashing (which is done by OEM not end users in the case of these boards), then MAC address would become generic as per packed in rom GBE.

Also see linuxboot/heads-wiki#209 (needed)

[FckBigTch]

I've revised the menu now. Instead of duplicating the whole function for the gated/non-gated case, the menu items are now built into a bash array, with the MAC randomization entry conditionally appended only when CONFIG_NVMUTIL=y and CONFIG_IFDTOOL=y.

[tlaurion]

Took a little time to think about linuxboot/heads-wiki#209 and current widespread

• CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal"

It seems that the proper usage pattern would become, in all boards having GBE

• CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd" as v540tu/v560tu
• And then, when calling flash.sh for GBE persistence when MAC randomized, save current CONFIG_FLASH_OPTION to BACKUP_FLASH_OPTION (BACKUP_FLASH_OPTION=CONFIG_FLASH_OPTION), append GBE region for current flash op (CONFIG_FLASH_OPTION+=" -i gbe"), flash, and then restore CONFIG_FLASH_OPTION=BACKUP_FLASH_OPTION).

Thoughts @FckBigTch ?

[tlaurion]

The reason I thought the Novacustom models should be excluded is that the config explicitly states:
"NovaCustom V54 MTL (integrated graphics) board configuration
Note the FLASH_OPTIONS: '--ifd -i bios -i me -i fd'
This excludes gbe from internal flashing, otherwise mac address would revert to '88:88:88:88:87:88' see #1871 (comment)
Same options should be used when externally flashing the first time, otherwise Intel GbE region (Ethernet config blob) will be overwritten and MAC reverted to '88:88:88:88:87:88'.”
That means it's not possible to generate a random MAC address, right? But I understand that overwriting the GBE region should be avoided during an update.

Misunderstanding. This means those boards pack a GBE (see coreboot configs) but don't ever flash it per initial flashing instructions or internal flashing. If someone misread initial flashing (which is done by OEM not end users in the case of these boards), then MAC address would become generic as per packed in rom GBE.

I hadn't even thought of the idea with the dynamic menu. I'll follow your advice and try to implement it.

Sorry i misled you @FckBigTch

grep -Ri gbe config/coreboot-* | grep nova
config/coreboot-novacustom-v540tu.config:CONFIG_MAINBOARD_USES_IFD_GBE_REGION=y
config/coreboot-novacustom-v540tu.config:# CONFIG_HAVE_GBE_BIN is not set
config/coreboot-novacustom-v560tu.config:CONFIG_MAINBOARD_USES_IFD_GBE_REGION=y
config/coreboot-novacustom-v560tu.config:# CONFIG_HAVE_GBE_BIN is not set

So, what that means is that the SPI chip HAS a GBE, but shipped ROM images don't and keep OEM's.
So. with IFDTOOL, we can get OEM unique GBE, modify it, flash it back.
The old "maximized" builds mindset was to put all required blobs (including config ones), where novacustom's took a different direction, which is arguably good but changed philosiphy as pointed in old heads-wiki doc so that initial flashing, and internal flashing, do not touch the region. Prior attempts to not pack GBE under x230 resulted in GBE not working correctly (we were overwritting GBE region by flashing whole SPI without one!), as well as current m900 workstation community effort not having working Ethernet, which i didn't investigate yet: but the m900 flashes with a backed GBE which for unknown reasons don't work.

So TLDR: We should have all boards having GBE only write to specific IFD's regions, not touching GBE even if one is provided by rom (excluding GBE from flashprog). And have GBE flashed when user intends it (when one is generated from nvmutil in this PR), where internal upgrade don't touch it unless expressively asked for. Makes sense?

Outside of v540tu/v560tu not having GBE in packed ROM; Others have it:

user@debian-13:~/heads-clean2$ grep -Ri "CONFIG_HAVE_GBE_BIN=y" config/coreboot-* | grep "\.config:"
config/coreboot-m900-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-optiplex-7019_9010-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-optiplex-7019_9010_TXT-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t420.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t420-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t430-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t440p.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t480-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t480s-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t520-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t530-dgpu-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t530-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w530-dgpu-K1000m-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w530-dgpu-K2000m-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w530-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w541.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x220.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x220-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x230-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x230-maximized-fhd_edp.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-z220-cmt.config:CONFIG_HAVE_GBE_BIN=y

Makes sense?

[FckBigTch]

Took a little time to think about linuxboot/heads-wiki#209 and current widespread

* `CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal"`

It seems that the proper usage pattern would become, in all boards having GBE

* `CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd"` as v540tu/v560tu

* And then, when calling flash.sh for GBE persistence when MAC randomized, save current CONFIG_FLASH_OPTION to BACKUP_FLASH_OPTION  (BACKUP_FLASH_OPTION=CONFIG_FLASH_OPTION), append GBE region for current flash op (CONFIG_FLASH_OPTION+="  -i gbe"), flash, and then restore CONFIG_FLASH_OPTION=BACKUP_FLASH_OPTION).

Thoughts @FckBigTch ?

I'm sorry for the late reply, but I had to think about what you wrote.
I’m still confused, not sure understanding you correctly.
Makes sense to me, defaulting all GBE-capable boards to exclude GBE from normal flash/update operations.
What I do not understand, why do you want to backup CONFIG_FLASH_OPTIONS?
You’re not just talking about MAC-randomization, but about flashing the gbe in general during an update or reflash?

If the user or whoever first did the (external) flash overwrote the GBE, so it’s DE:AD:CO:FF:EE. If they use the randomization feature, they get a random MAC. If all boards exclude GBE (CONFIG_FLASH_OPTIONS change), the random mac is persistent during an update.
Do you want to give the user the option to have the GBE overwritten again during an update or reflash so that they get DE:AD:CO:FF:EE back?

I'm not sure if that's really necessary, but I understand that you want to offer an easy way to revert to the default settings.

[FckBigTch]

The reason I thought the Novacustom models should be excluded is that the config explicitly states:
"NovaCustom V54 MTL (integrated graphics) board configuration
Note the FLASH_OPTIONS: '--ifd -i bios -i me -i fd'
This excludes gbe from internal flashing, otherwise mac address would revert to '88:88:88:88:87:88' see #1871 (comment)
Same options should be used when externally flashing the first time, otherwise Intel GbE region (Ethernet config blob) will be overwritten and MAC reverted to '88:88:88:88:87:88'.”
That means it's not possible to generate a random MAC address, right? But I understand that overwriting the GBE region should be avoided during an update.

Misunderstanding. This means those boards pack a GBE (see coreboot configs) but don't ever flash it per initial flashing instructions or internal flashing. If someone misread initial flashing (which is done by OEM not end users in the case of these boards), then MAC address would become generic as per packed in rom GBE.

I hadn't even thought of the idea with the dynamic menu. I'll follow your advice and try to implement it.

Sorry i misled you @FckBigTch

grep -Ri gbe config/coreboot-* | grep nova
config/coreboot-novacustom-v540tu.config:CONFIG_MAINBOARD_USES_IFD_GBE_REGION=y
config/coreboot-novacustom-v540tu.config:# CONFIG_HAVE_GBE_BIN is not set
config/coreboot-novacustom-v560tu.config:CONFIG_MAINBOARD_USES_IFD_GBE_REGION=y
config/coreboot-novacustom-v560tu.config:# CONFIG_HAVE_GBE_BIN is not set

So, what that means is that the SPI chip HAS a GBE, but shipped ROM images don't and keep OEM's. So. with IFDTOOL, we can get OEM unique GBE, modify it, flash it back. The old "maximized" builds mindset was to put all required blobs (including config ones), where novacustom's took a different direction, which is arguably good but changed philosiphy as pointed in old heads-wiki doc so that initial flashing, and internal flashing, do not touch the region. Prior attempts to not pack GBE under x230 resulted in GBE not working correctly (we were overwritting GBE region by flashing whole SPI without one!), as well as current m900 workstation community effort not having working Ethernet, which i didn't investigate yet: but the m900 flashes with a backed GBE which for unknown reasons don't work.

So TLDR: We should have all boards having GBE only write to specific IFD's regions, not touching GBE even if one is provided by rom (excluding GBE from flashprog). And have GBE flashed when user intends it (when one is generated from nvmutil in this PR), where internal upgrade don't touch it unless expressively asked for. Makes sense?

Outside of v540tu/v560tu not having GBE in packed ROM; Others have it:

user@debian-13:~/heads-clean2$ grep -Ri "CONFIG_HAVE_GBE_BIN=y" config/coreboot-* | grep "\.config:"
config/coreboot-m900-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-optiplex-7019_9010-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-optiplex-7019_9010_TXT-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t420.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t420-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t430-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t440p.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t480-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t480s-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t520-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t530-dgpu-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-t530-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w530-dgpu-K1000m-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w530-dgpu-K2000m-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w530-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-w541.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x220.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x220-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x230-maximized.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-x230-maximized-fhd_edp.config:CONFIG_HAVE_GBE_BIN=y
config/coreboot-z220-cmt.config:CONFIG_HAVE_GBE_BIN=y

Makes sense?

So, is there anything else you'd like me to do?
I can make the changes to CONFIG_FLASH_OPTIONS for all relevant boards like this
CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd

[tlaurion]

Took a little time to think about linuxboot/heads-wiki#209 and current widespread

* `CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal"`

It seems that the proper usage pattern would become, in all boards having GBE

* `CONFIG_FLASH_OPTIONS="flashprog --progress --programmer internal --ifd -i bios -i me -i fd"` as v540tu/v560tu

* And then, when calling flash.sh for GBE persistence when MAC randomized, save current CONFIG_FLASH_OPTION to BACKUP_FLASH_OPTION  (BACKUP_FLASH_OPTION=CONFIG_FLASH_OPTION), append GBE region for current flash op (CONFIG_FLASH_OPTION+="  -i gbe"), flash, and then restore CONFIG_FLASH_OPTION=BACKUP_FLASH_OPTION).

Thoughts @FckBigTch ?

I'm sorry for the late reply, but I had to think about what you wrote. I’m still confused, not sure understanding you correctly. Makes sense to me, defaulting all GBE-capable boards to exclude GBE from normal flash/update operations. What I do not understand, why do you want to backup CONFIG_FLASH_OPTIONS? You’re not just talking about MAC-randomization, but about flashing the gbe in general during an update or reflash?

If the user or whoever first did the (external) flash overwrote the GBE, so it’s DE:AD:CO:FF:EE. If they use the randomization feature, they get a random MAC. If all boards exclude GBE (CONFIG_FLASH_OPTIONS change), the random mac is persistent during an update. Do you want to give the user the option to have the GBE overwritten again during an update or reflash so that they get DE:AD:CO:FF:EE back?

I'm not sure if that's really necessary, but I understand that you want to offer an easy way to revert to the default settings.

Sorry for having confused you. I checked the changes, and now I understand why the misunderstandings.
Everywhere in codebase, flashing operations use flash.sh wrapper which in turn uses CONFIG_FLASH_OPTIONS; you call flashprog directly. This is not a good pattern since if/when flashrom will have proper --progress support to inform use of progress of flashing operations, we might switch back to flashrom and calling the packed programmer directly is not what the codebase do without verifying its existence. flash.sh and CONFIG_FLASH_OPTIONS are used everywhere in codebase for that reason.

Once this is CONFIG_FLASH_OPTIONS and flash.sh used, why I wanted to keep prior CONFIG_FLASH_OPTIONS to restore it before returning to prior operations is exposed, otherwise a reboot.sh would be needed (also normal pattern after a flash. We might need to simply add in all boards CONFIG_FLASH_TOOL="flashprog" and CONFIG_FLASH_OPTIONS=$CONFIG_FLASH_TOOL --programmer....." so that you could use CONFIG_FLASH directly if that is the path you prefer without needing backup of previous board, extract the programmer to call only --ifd -i gbe... That's where I was going to.

gui-init.sh should not have additional functions, it should either be in functions.sh or gui_functions.sh that gui-init.sh sources. One cannot cd into /tmp in one function and not change back to prior location either since other heads workflows will break, unless a reboot is done too there. Some of your added functions do not check for ifdtool or nvutil presence (not guarded) which might also cause issues down the line which we try to not have to deal with.

Sorry I wasn't clear enough; I didn't see you were calling flashprog directly in added functions under gui-init.sh

[FckBigTch]

Make sense! Thanks for your patience and explanations. I will make the changes.