Change in ...coreboot[master]: intel/apollolake: Add parameter to enable VTD in devicetree

[Werner Zeh (Code Review)]

Werner Zeh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31194

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

intel/apollolake: Add parameter to enable VTD in devicetree

The FSP has a parameter to enable or disable the VTD feature. VTD is
disabled per default. Add a chip parameter so that VTD can be enabled on
mainboard level in devicetree.

Change-Id: Ic0bfcf1719e1ccc678a932bf3d38c6dbce3556bc
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/31194/1

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index cddfe44..735fed0 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -751,6 +751,9 @@
    if (!xdci_can_enable())
    	dev->enabled = 0;
    silconfig->UsbOtg = dev->enabled;
+
+	/* Set VTD feature according to devicetree */
+	silconfig->VtdEnable = cfg->enable_vtd;
 }

struct chip_operations soc_intel_apollolake_ops = {
diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h
index 6c2404a..b9e368c 100644
--- a/src/soc/intel/apollolake/chip.h
+++ b/src/soc/intel/apollolake/chip.h
@@ -188,6 +188,12 @@
     * 00=1.10v, 01=1.15v, 10=1.24v, 11=1.20v (default).
     */
    uint32_t PmicVdd2Voltage;
+
+	/* Option to enable VTD feature. Default is 0 which disables VTD
+	 * capability in FSP. Setting this option to 1 in devicetree will enable
+	 * the Upd parameter VtdEnable.
+	 */
+	uint8_t enable_vtd;
 };

typedef struct soc_intel_apollolake_config config_t;

[Nico Huber (Code Review)]

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

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 1: Code-Review+1

(2 comments)

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h@196 
PS1, Line 196: 	uint8_t enable_vtd;
Newer platforms have a vtd_disable. Maybe normalize on that?

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.c 
File src/soc/intel/apollolake/chip.c:

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.c@756 
PS1, Line 756: 	silconfig->VtdEnable = cfg->enable_vtd;
This might affect all APL/GLK boards. Before this change, `VtdEnable`
was set by the FSP binary, after, by the (absence of the) devicetree
setting.

[Aaron Durbin (Code Review)]

Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31194 )

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 1:

What was the default UPD setting in glk and apl fsp? I'm ok with the change, but we should be informed of what the previous defaults were.

[Paul Menzel (Code Review)]

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

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 1: Code-Review+1

(1 comment)

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h@192 
PS1, Line 192: VTD
Please add a quick explanation, what VTD is in the comment.

[Nico Huber (Code Review)]

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

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 1:
> What was the default UPD setting in glk and apl fsp? I'm ok with the change, but we should be informed of what the previous defaults were.

Current header files indicate a default of 0, i.e. disabled.
Werner, please mention this in the commit message.

There is no guarantee, though, that all binaries have the
same defaults.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31194 )

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 1:
> Patch Set 1:
> > What was the default UPD setting in glk and apl fsp? I'm ok with the change, but we should be informed of what the previous defaults were.
>
> Current header files indicate a default of 0, i.e. disabled.
> Werner, please mention this in the commit message.
>
> There is no guarantee, though, that all binaries have the
> same defaults.

Thanks Nico for stepping in. As you already have depicted, current header files for both Apollo Lake and Gemini Lake indicates, that Vtd is disabled per default:

/** Offset 0x026A - VT-d
  Enable/Disable VT-d. 0:Disable(Default), 1:Enable.
  $EN_DIS
**/
  UINT8                       VtdEnable;

We still might hit the case where the FSP has been modified via BCT binary and this default is not true anymore. In this case we might end up with disabled Vtd feature as it is not mentioned in the mainboard's devicetree. But on the other hand, Vtd is only fully useful if one have DMAR table around for the OS to handle this kind of virtualization. And the generation of this DMAR table has been added a few weeks ago only. So I doubt that this case, where FSP has been modified via BCT should happen too often (if at all).

I will add a better description to the commit message.

[Werner Zeh (Code Review)]

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31194 )

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 1:

(2 comments)

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h@192 
PS1, Line 192: VTD
> Please add a quick explanation, what VTD is in the comment.

Will add the description in the commit message.

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h@196 
PS1, Line 196: 	uint8_t enable_vtd;
> Newer platforms have a vtd_disable. […]

Where have you seen it in the tree? The only thing I can find like "vtd_disable" is this one bit in the CAPID_A register. This has a slightly different meaning as it dos not target a FSP parameter.
If I switch the logic to "vtd_disable", then we will have a conflict with the default (so to speak not set) devicetree value resulting in the need of setting it on all APL+GLK based mainboards.
Or did I get you wrong?

[Werner Zeh (Code Review)]

Hello Aaron Durbin, Patrick Rudolph, Subrata Banik, Paul Menzel, build bot (Jenkins), Nico Huber,

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

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

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

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

intel/apollolake: Add parameter to enable VTD in devicetree

The FSP has a parameter to enable or disable the VTD feature
(Intel's Virtualization Technology for Directed I/O). In current header
files for FSP-S (Apollo Lake and Gemini Lake) this parameter is set to
disabled per default. Therefore, if the FSP was not modified via BCT,
this feature is most likely disabled on all mainboards.

Add a chip parameter so that VTD can be enabled on mainboard level in
devicetree and therefore this feature can be activated if needed.

Change-Id: Ic0bfcf1719e1ccc678a932bf3d38c6dbce3556bc
Signed-off-by: Werner Zeh 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/94/31194/2

[Furquan Shaikh (Code Review)]

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

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 2: Code-Review+2

[Nico Huber (Code Review)]

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

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

Patch Set 2: Code-Review+2

(1 comment)

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h 
File src/soc/intel/apollolake/chip.h:

https://review.coreboot.org/#/c/31194/1/src/soc/intel/apollolake/chip.h@196 
PS1, Line 196: 	uint8_t enable_vtd;
> Where have you seen it in the tree? The only thing I can find like "vtd_disable" is this one bit in  […]

A Freudian slip, maybe. They call it `VtdDisable`. And I've
looked at it more closely now, it seems it's not newer plat-
forms but all the big-core ones.

[Werner Zeh (Code Review)]

Werner Zeh has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31194 )

Change subject: intel/apollolake: Add parameter to enable VTD in devicetree
......................................................................

intel/apollolake: Add parameter to enable VTD in devicetree

The FSP has a parameter to enable or disable the VTD feature
(Intel's Virtualization Technology for Directed I/O). In current header
files for FSP-S (Apollo Lake and Gemini Lake) this parameter is set to
disabled per default. Therefore, if the FSP was not modified via BCT,
this feature is most likely disabled on all mainboards.

Add a chip parameter so that VTD can be enabled on mainboard level in
devicetree and therefore this feature can be activated if needed.

Change-Id: Ic0bfcf1719e1ccc678a932bf3d38c6dbce3556bc
Signed-off-by: Werner Zeh 
Reviewed-on: https://review.coreboot.org/c/31194
Reviewed-by: Furquan Shaikh 
Reviewed-by: Nico Huber 
Reviewed-by: Paul Menzel 
Tested-by: build bot (Jenkins) 
---
M src/soc/intel/apollolake/chip.c
M src/soc/intel/apollolake/chip.h
2 files changed, 9 insertions(+), 0 deletions(-)

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

diff --git a/src/soc/intel/apollolake/chip.c b/src/soc/intel/apollolake/chip.c
index de33e82..3634509 100644
--- a/src/soc/intel/apollolake/chip.c
+++ b/src/soc/intel/apollolake/chip.c
@@ -754,6 +754,9 @@
    if (!xdci_can_enable())
    	dev->enabled = 0;
    silconfig->UsbOtg = dev->enabled;
+
+	/* Set VTD feature according to devicetree */
+	silconfig->VtdEnable = cfg->enable_vtd;
 }

struct chip_operations soc_intel_apollolake_ops = {
diff --git a/src/soc/intel/apollolake/chip.h b/src/soc/intel/apollolake/chip.h
index 6c2404a..b9e368c 100644
--- a/src/soc/intel/apollolake/chip.h
+++ b/src/soc/intel/apollolake/chip.h
@@ -188,6 +188,12 @@
     * 00=1.10v, 01=1.15v, 10=1.24v, 11=1.20v (default).
     */
    uint32_t PmicVdd2Voltage;
+
+	/* Option to enable VTD feature. Default is 0 which disables VTD
+	 * capability in FSP. Setting this option to 1 in devicetree will enable
+	 * the Upd parameter VtdEnable.
+	 */
+	uint8_t enable_vtd;
 };

typedef struct soc_intel_apollolake_config config_t;