Change in ...coreboot[master]: soc/intel/cannonlake: select VMX configurabiltity

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31442

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

soc/intel/cannonlake: select VMX configurabiltity

Select VMX configuration config and implement required soc side API.
Also, run vmx_configure() on all CPUs

BUG=b:124518711

Change-Id: If9a49deb52e33c1c400a7e2cfab6337b62ecc08e
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/cpu.c
2 files changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/31442/1

diff --git a/src/soc/intel/cannonlake/Kconfig b/src/soc/intel/cannonlake/Kconfig
index cd8819d..bacbe7b 100644
--- a/src/soc/intel/cannonlake/Kconfig
+++ b/src/soc/intel/cannonlake/Kconfig
@@ -85,6 +85,7 @@
    select SOC_INTEL_COMMON_BLOCK_SA
    select SOC_INTEL_COMMON_BLOCK_SMM
    select SOC_INTEL_COMMON_BLOCK_SMM_IO_TRAP
+	select SOC_INTEL_COMMON_BLOCK_VMX
    select SOC_INTEL_COMMON_PCH_BASE
    select SOC_INTEL_COMMON_NHLT
    select SOC_INTEL_COMMON_RESET
diff --git a/src/soc/intel/cannonlake/cpu.c b/src/soc/intel/cannonlake/cpu.c
index f987f8b..5f73681 100644
--- a/src/soc/intel/cannonlake/cpu.c
+++ b/src/soc/intel/cannonlake/cpu.c
@@ -24,6 +24,7 @@
 #include <intelblocks/cpulib.h>
 #include <intelblocks/mp_init.h>
 #include <intelblocks/smm.h>
+#include <intelblocks/vmx.h>
 #include <romstage_handoff.h>
 #include <soc/cpu.h>
 #include <soc/msr.h>
@@ -31,6 +32,7 @@
 #include <soc/pm.h>
 #include <soc/smm.h>
 #include <soc/systemagent.h>
+#include <timer.h>

/* Convert time in seconds to POWER_LIMIT_1_TIME MSR value */
 static const u8 power_limit_time_sec_to_msr[] = {
@@ -458,6 +460,8 @@

/* Lock down the SMRAM space. */
    smm_lock();
+
+	mp_run_on_all_cpus(vmx_configure, NULL, 2 * USECS_PER_MSEC);
 }

static const struct mp_ops mp_ops = {
@@ -484,3 +488,23 @@
    /* Thermal throttle activation offset */
    configure_thermal_target();
 }
+
+int soc_fill_vmx_param(struct vmx_param *vmx_param)
+{
+	struct device *dev = SA_DEV_ROOT;
+	config_t *conf;
+
+	if (!dev) {
+		printk(BIOS_ERR, "Failed to get root dev for checking VMX param\n");
+		return -1;
+	}
+
+	conf = dev->chip_info;
+	if (!conf) {
+		printk(BIOS_ERR, "Failed to get chip_info for VMX param\n");
+		return -1;
+	}
+
+	vmx_param->enable = conf->VmxEnable;
+	return 0;
+}

[Rizwan Qureshi (Code Review)]

Hello Patrick Rudolph, Subrata Banik, Duncan Laurie, Furquan Shaikh,

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

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

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

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

soc/intel/cannonlake: select VMX configurabiltity

Select VMX config and implement required soc side API.
Also, run vmx_configure() on all CPUs

BUG=b:124518711

Change-Id: If9a49deb52e33c1c400a7e2cfab6337b62ecc08e
Signed-off-by: Rizwan Qureshi 
---
M src/soc/intel/cannonlake/Kconfig
M src/soc/intel/cannonlake/cpu.c
2 files changed, 25 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/31442/2

[Arthur Heymans (Code Review)]

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31442 )

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31442/2/src/soc/intel/cannonlake/cpu.c 
File src/soc/intel/cannonlake/cpu.c:

https://review.coreboot.org/#/c/31442/2/src/soc/intel/cannonlake/cpu.c@464 
PS2, Line 464: mp_run_on_all_cpus(vmx_configure, NULL, 2 * USECS_PER_MSEC);
             : }
could you explain in the commit message and here in the code why this is run here and not simply during cpu_driver ops? Also why is the wheel reinvented with vmx_configure in soc/intel while there has been code that does the more or less the same in cpu/intel/common?

[Nico Huber (Code Review)]

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

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/#/c/31442/2/src/soc/intel/cannonlake/cpu.c 
File src/soc/intel/cannonlake/cpu.c:

https://review.coreboot.org/#/c/31442/2/src/soc/intel/cannonlake/cpu.c@464 
PS2, Line 464: mp_run_on_all_cpus(vmx_configure, NULL, 2 * USECS_PER_MSEC);
             : }
> could you explain in the commit message and here in the code why this is run here and not simply dur […]

Ah, yes, we were about to abandon SOC_INTEL_COMMON_BLOCK_VMX in favor
of CPU_COMMON (CB:29683). You'd simply have to call set_feature_ctrl_vmx()
instead. Also set_feature_ctrl_lock() eventually. Both on all cores but
doesn't have to happen in post_mp_init(). The locking step, of course,
after all other changes to IA32_FEATURE_CONTROL.

We have been using a Kconfig setting to toggle VMX all the time. So a
devicetree setting is unnecessary.

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

Patch Set 2:

Do you want to rebase your change to align with the common code?

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31442 )

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

Patch Set 2:
> Patch Set 2:
>
> Do you want to rebase your change to align with the common code?

Yes, will spin a new patchset

[Furquan Shaikh (Code Review)]

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

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

Patch Set 2:
> Patch Set 2:
> > Patch Set 2:
> >
> > Do you want to rebase your change to align with the common code?
>
> Yes, will spin a new patchset

Any update on this?

[Rizwan Qureshi (Code Review)]

Rizwan Qureshi has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/31442 )

Change subject: soc/intel/cannonlake: select VMX configurabiltity
......................................................................

Abandoned

abandoning in lieu of https://review.coreboot.org/c/coreboot/+/31928