Change in coreboot[master]: soc/intel/common/block/xdci: Add override for vboot policy

[Wim Vervoorn (Code Review)]

Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: soc/intel/common/block/xdci: Add override for vboot policy
......................................................................

soc/intel/common/block/xdci: Add override for vboot policy

When VBOOT is enabled the xDCI controller can only be enabled in
developer mode. This is not neccessarily correct for any given system
with VBOOT enabled.

Add the possibility to override the vboot policy. So the xDCI controller
will be enabled when it is enabled in the device tree.

BUG=N/A
TEST=tested on facebook monolith

Change-Id: I6142c4a74ca6930457b16f62f32e1199b8baaff8
Signed-off-by: Wim Vervoorn 
---
M src/mainboard/facebook/monolith/Kconfig
M src/soc/intel/common/block/xdci/Kconfig
M src/soc/intel/common/block/xdci/xdci.c
3 files changed, 10 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38403/1

diff --git a/src/mainboard/facebook/monolith/Kconfig b/src/mainboard/facebook/monolith/Kconfig
index 3261661..93e307b 100644
--- a/src/mainboard/facebook/monolith/Kconfig
+++ b/src/mainboard/facebook/monolith/Kconfig
@@ -11,6 +11,7 @@
    select MAINBOARD_HAS_TPM2
    select MAINBOARD_USES_IFD_GBE_REGION
    select INTEL_GMA_HAVE_VBT
+	select XDCI_VBOOT_FORCE_ENABLE

config CBFS_SIZE
    hex "CBFS_SIZE"
diff --git a/src/soc/intel/common/block/xdci/Kconfig b/src/soc/intel/common/block/xdci/Kconfig
index 3918642..b613417 100644
--- a/src/soc/intel/common/block/xdci/Kconfig
+++ b/src/soc/intel/common/block/xdci/Kconfig
@@ -2,3 +2,10 @@
    bool
    help
      Intel Processor common XDCI support
+
+config XDCI_VBOOT_FORCE_ENABLE
+	bool
+	depends on SOC_INTEL_COMMON_BLOCK_XDCI
+	help
+	  Overrides the VBOOT UDC policy and enables
+	  XDCI when enabled in the device tree
diff --git a/src/soc/intel/common/block/xdci/xdci.c b/src/soc/intel/common/block/xdci/xdci.c
index 2296f9f..39397f9 100644
--- a/src/soc/intel/common/block/xdci/xdci.c
+++ b/src/soc/intel/common/block/xdci/xdci.c
@@ -24,6 +24,8 @@

int xdci_can_enable(void)
 {
+	if (CONFIG(XDCI_VBOOT_FORCE_ENABLE))
+		return 1;
    return vboot_can_enable_udc();
 }

[Paul Menzel (Code Review)]

Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: soc/intel/common/block/xdci: Add override for vboot policy
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/38403/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/xdci/Kconfig:

https://review.coreboot.org/c/coreboot/+/38403/2/src/soc/intel/common/block/... 
PS2, Line 11: 	  XDCI when enabled in the device tree
More of that fits in the first line. Please add a dot/period at the end of the sentence.

[Wim Vervoorn (Code Review)]

Hello Patrick Rudolph, Frans Hendriks, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38403

to look at the new patch set (#3).

Change subject: soc/intel/common/block/xdci: Add override for vboot policy
......................................................................

soc/intel/common/block/xdci: Add override for vboot policy

When VBOOT is enabled the xDCI controller can only be enabled in
developer mode. This is not neccessarily correct for any given system
with VBOOT enabled.

Add the possibility to override the vboot policy. So the xDCI controller
will be enabled when it is enabled in the device tree.

BUG=N/A
TEST=tested on facebook monolith

Change-Id: I6142c4a74ca6930457b16f62f32e1199b8baaff8
Signed-off-by: Wim Vervoorn 
---
M src/mainboard/facebook/monolith/Kconfig
M src/soc/intel/common/block/xdci/Kconfig
M src/soc/intel/common/block/xdci/xdci.c
3 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38403/3

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: soc/intel/common/block/xdci: Add override for vboot policy
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/38403/2/src/soc/intel/common/block/... 
File src/soc/intel/common/block/xdci/Kconfig:

https://review.coreboot.org/c/coreboot/+/38403/2/src/soc/intel/common/block/... 
PS2, Line 11: 	  XDCI when enabled in the device tree
> More of that fits in the first line. Please add a dot/period at the end of the sentence.

Done

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: soc/intel/common/block/xdci: Add override for vboot policy
......................................................................

Patch Set 3: Code-Review+2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: soc/intel/common/block/xdci: Add override for vboot policy
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38403/3/src/mainboard/facebook/mono... 
File src/mainboard/facebook/monolith/Kconfig:

PS3: 
Can you please add mainboard changes in a separate CL?

https://review.coreboot.org/c/coreboot/+/38403/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/xdci/Kconfig:

https://review.coreboot.org/c/coreboot/+/38403/3/src/soc/intel/common/block/... 
PS3, Line 6: XDCI_VBOOT_FORCE_ENABLE
How about adding a config to src/security/vboot/Kconfig "VBOOT_ALWAYS_ALLOW_XDCI" which can be checked by vboot_can_enable_udc() to decide if XDCI should be enabled:

diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c
index 458ed87982a..5e7282323a0 100644
--- a/src/security/vboot/vboot_common.c
+++ b/src/security/vboot/vboot_common.c
@@ -27,6 +27,10 @@
 /* Check if it is okay to enable USB Device Controller (UDC). */
 int vboot_can_enable_udc(void)
 {
+       /* Allow XHCI in all vboot modes. */
+       if (CONFIG(VBOOT_ALWAYS_ALLOW_XDCI))
+               return 1;
+
        /* Always disable if not in developer mode */
        if (!vboot_developer_mode_enabled())
                return 0;

This will help other vboot platforms too in case they care about implementing this policy. Also, this config can be auto-selected if !CHROMEOS since currently CHROMEOS is probably the only one that cares about disabling XDCI in non-developer mode.

Change in coreboot[master]: security/vboot: Allow UDC regardless of vboot state

[Wim Vervoorn (Code Review)]

Hello Patrick Rudolph, Julius Werner, Frans Hendriks, Paul Menzel, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/38403

to look at the new patch set (#4).

Change subject: security/vboot: Allow UDC regardless of vboot state
......................................................................

security/vboot: Allow UDC regardless of vboot state

When a VBOOT enabled system is used without ChromeOS it may be valid to
allow the UDC independent of the vboot state.

Provide the option to always allow UDC when CHROMEOS is not selected.

BUG=N/A
TEST=build

Change-Id: I6142c4a74ca6930457b16f62f32e1199b8baaff8
Signed-off-by: Wim Vervoorn 
---
M src/security/vboot/Kconfig
M src/security/vboot/vboot_common.c
2 files changed, 11 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/03/38403/4

Change in coreboot[master]: security/vboot: Allow UDC regardless of vboot state

[Wim Vervoorn (Code Review)]

Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: security/vboot: Allow UDC regardless of vboot state
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/c/coreboot/+/38403/3/src/mainboard/facebook/mono... 
File src/mainboard/facebook/monolith/Kconfig:

PS3:
> Can you please add mainboard changes in a separate CL?

Done

https://review.coreboot.org/c/coreboot/+/38403/3/src/soc/intel/common/block/... 
File src/soc/intel/common/block/xdci/Kconfig:

https://review.coreboot.org/c/coreboot/+/38403/3/src/soc/intel/common/block/... 
PS3, Line 6: XDCI_VBOOT_FORCE_ENABLE
> How about adding a config to src/security/vboot/Kconfig "VBOOT_ALWAYS_ALLOW_XDCI" which can be check […]

Good suggestion. Just updated the patch. This is indeed much more generic. I have defaulted the option to N to make sure I don't break expected behavior on other systems. The option can be selected on board level like I did for the monolith system.

The user can only set the option when CHROMEOS is not enabled. This is done to make sure the CHROMEOS expectations for the UDC enabled can't be corrupted by a configuration mistake.

Change in coreboot[master]: security/vboot: Allow UDC regardless of vboot state

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: security/vboot: Allow UDC regardless of vboot state
......................................................................

Patch Set 4: Code-Review+1

Change in coreboot[master]: security/vboot: Allow UDC regardless of vboot state

[Frans Hendriks (Code Review)]

Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: security/vboot: Allow UDC regardless of vboot state
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: security/vboot: Allow UDC regardless of vboot state

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: security/vboot: Allow UDC regardless of vboot state
......................................................................

Patch Set 4: Code-Review+2

Change in coreboot[master]: security/vboot: Allow UDC regardless of vboot state

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38403 )

Change subject: security/vboot: Allow UDC regardless of vboot state
......................................................................

security/vboot: Allow UDC regardless of vboot state

When a VBOOT enabled system is used without ChromeOS it may be valid to
allow the UDC independent of the vboot state.

Provide the option to always allow UDC when CHROMEOS is not selected.

BUG=N/A
TEST=build

Change-Id: I6142c4a74ca6930457b16f62f32e1199b8baaff8
Signed-off-by: Wim Vervoorn 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/38403
Tested-by: build bot (Jenkins) 
Reviewed-by: Nico Huber 
Reviewed-by: Frans Hendriks 
Reviewed-by: Furquan Shaikh 
---
M src/security/vboot/Kconfig
M src/security/vboot/vboot_common.c
2 files changed, 11 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Nico Huber: Looks good to me, but someone else must approve
  Furquan Shaikh: Looks good to me, approved
  Frans Hendriks: Looks good to me, approved

diff --git a/src/security/vboot/Kconfig b/src/security/vboot/Kconfig
index 787cdbe..7e86c7c 100644
--- a/src/security/vboot/Kconfig
+++ b/src/security/vboot/Kconfig
@@ -181,6 +181,13 @@
    help
      Set this option to indicate to vboot that display should always be enabled.

+config VBOOT_ALWAYS_ALLOW_UDC
+	bool "Always allow UDC"
+	default n
+	depends on !CHROMEOS
+	help
+	  This option allows UDC to be enabled regardless of the vboot state.
+
 config VBOOT_HAS_REC_HASH_SPACE
    bool
    default n
diff --git a/src/security/vboot/vboot_common.c b/src/security/vboot/vboot_common.c
index 458ed87..3342524 100644
--- a/src/security/vboot/vboot_common.c
+++ b/src/security/vboot/vboot_common.c
@@ -27,6 +27,10 @@
 /* Check if it is okay to enable USB Device Controller (UDC). */
 int vboot_can_enable_udc(void)
 {
+	/* Allow UDC in all vboot modes. */
+	if (!CONFIG(CHROMEOS) && CONFIG(VBOOT_ALWAYS_ALLOW_UDC))
+		return 1;
+
    /* Always disable if not in developer mode */
    if (!vboot_developer_mode_enabled())
    	return 0;