[3/3] lib/fwts_cpu: use new use the new module loading helper functions
diff mbox series

Message ID 20181108144511.12678-4-colin.king@canonical.com
State Accepted
Headers show
Series
  • Remove modprobe, replace with module system calls
Related show

Commit Message

Colin King Nov. 8, 2018, 2:45 p.m. UTC
From: Colin Ian King <colin.king@canonical.com>

Make the msr library helper now use the new module loading
functions. This involves adding an extra fw parameter which means
msr releated tests need modifying too.

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/bios/mtrr/mtrr.c       |  7 ++++---
 src/cpu/msr/msr.c          | 14 ++++++++------
 src/cpu/nx/nx.c            |  2 +-
 src/cpu/virt/virt_svm.c    |  6 +++---
 src/cpu/virt/virt_vmx.c    |  6 +++---
 src/lib/include/fwts_cpu.h |  2 +-
 src/lib/src/fwts_cpu.c     | 22 ++++++++++++++++------
 7 files changed, 36 insertions(+), 23 deletions(-)

Comments

Alex Hung Nov. 13, 2018, 7:03 a.m. UTC | #1
On 2018-11-08 10:45 p.m., Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Make the msr library helper now use the new module loading
> functions. This involves adding an extra fw parameter which means
> msr releated tests need modifying too.
> 
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/bios/mtrr/mtrr.c       |  7 ++++---
>  src/cpu/msr/msr.c          | 14 ++++++++------
>  src/cpu/nx/nx.c            |  2 +-
>  src/cpu/virt/virt_svm.c    |  6 +++---
>  src/cpu/virt/virt_vmx.c    |  6 +++---
>  src/lib/include/fwts_cpu.h |  2 +-
>  src/lib/src/fwts_cpu.c     | 22 ++++++++++++++++------
>  7 files changed, 36 insertions(+), 23 deletions(-)
> 
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 5dfb4a60..4948b55a 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -182,8 +182,9 @@ static int get_mtrrs(void)
>  	return FWTS_OK;
>  }
>  
> -static int get_default_mtrr(void) {
> -	if (fwts_cpu_readmsr(0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
> +static int get_default_mtrr(fwts_framework *fw)
> +{
> +	if (fwts_cpu_readmsr(fw, 0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
>  		switch (mtrr_default & 0xFF) {
>  			case 0:
>  				mtrr_default = UNCACHED;
> @@ -408,7 +409,7 @@ static int validate_iomem(fwts_framework *fw)
>  	if ((file = fopen("/proc/iomem", "r")) == NULL)
>  		return FWTS_ERROR;
>  
> -	if (get_default_mtrr() != FWTS_OK)
> +	if (get_default_mtrr(fw) != FWTS_OK)
>  		mtrr_default = UNKNOWN;
>  
>  	while (!feof(file)) {
> diff --git a/src/cpu/msr/msr.c b/src/cpu/msr/msr.c
> index 70d89650..a9dfd40c 100644
> --- a/src/cpu/msr/msr.c
> +++ b/src/cpu/msr/msr.c
> @@ -77,7 +77,9 @@ static int msr_deinit(fwts_framework *fw)
>  	return FWTS_OK;
>  }
>  
> -static int msr_consistent(const uint32_t msr,
> +static int msr_consistent(
> +	fwts_framework *fw,
> +	const uint32_t msr,
>  	const int shift,
>  	const uint64_t mask,
>  	uint64_t *const vals,
> @@ -90,7 +92,7 @@ static int msr_consistent(const uint32_t msr,
>  
>  	for (cpu = 0; cpu < ncpus; cpu++) {
>  		uint64_t val;
> -		if (fwts_cpu_readmsr(cpu, msr, &val) != FWTS_OK) {
> +		if (fwts_cpu_readmsr(fw, cpu, msr, &val) != FWTS_OK) {
>  			return FWTS_ERROR;
>  		}
>  		val >>= shift;
> @@ -130,7 +132,7 @@ static int msr_consistent_check(fwts_framework *fw,
>  		free(vals);
>  		return FWTS_ERROR;
>  	}
> -	if (msr_consistent(msr, shift, mask,
> +	if (msr_consistent(fw, msr, shift, mask,
>  		vals, &inconsistent_count, inconsistent) != FWTS_OK) {
>  		free(inconsistent);
>  		free(vals);
> @@ -197,7 +199,7 @@ static int msr_smrr(fwts_framework *fw)
>  	uint64_t val;
>  
>  	if (intel_cpu) {
> -		if (fwts_cpu_readmsr(0, 0xfe, &val) != FWTS_OK) {
> +		if (fwts_cpu_readmsr(fw, 0, 0xfe, &val) != FWTS_OK) {
>  			fwts_skipped(fw, "Cannot read MSR 0xfe.");
>  			return FWTS_ERROR;
>  		}
> @@ -210,7 +212,7 @@ static int msr_smrr(fwts_framework *fw)
>  			msr_consistent_check(fw, LOG_LEVEL_HIGH, "SMRR_PHYSMASK", 0x1f3, 12, 0xfffff, NULL);
>  			msr_consistent_check(fw, LOG_LEVEL_HIGH, "SMRR_VALID", 0x1f3, 11, 0x1, NULL);
>  
> -			if (fwts_cpu_readmsr(0, 0x1f2, &val) == FWTS_OK) {
> +			if (fwts_cpu_readmsr(fw, 0, 0x1f2, &val) == FWTS_OK) {
>  				uint64_t physbase = val & 0xfffff000;
>  				uint64_t type = val & 7;
>  				if ((physbase & 0xfff) != 0)
> @@ -221,7 +223,7 @@ static int msr_smrr(fwts_framework *fw)
>  					fwts_failed(fw, LOG_LEVEL_HIGH, "MSRSMRR_TYPE",
>  						"SMRR: SMRR_TYPE is 0x%" PRIx64 ", should be 0x6 (Write-Back).", type);
>  			}
> -			if (fwts_cpu_readmsr(0, 0x1f3, &val) == FWTS_OK) {
> +			if (fwts_cpu_readmsr(fw, 0, 0x1f3, &val) == FWTS_OK) {
>  				uint64_t physmask = val & 0xfffff000;
>  				uint64_t valid = (val >> 11) & 1;
>  
> diff --git a/src/cpu/nx/nx.c b/src/cpu/nx/nx.c
> index 66a7b6f1..5caf0c6c 100644
> --- a/src/cpu/nx/nx.c
> +++ b/src/cpu/nx/nx.c
> @@ -169,7 +169,7 @@ static int nx_test3(fwts_framework *fw)
>  			fwts_cpu_free_info(fwts_nx_cpuinfo);
>  			return FWTS_OK;
>  		}
> -		if (fwts_cpu_readmsr(i, 0x1a0, &val) != FWTS_OK) {
> +		if (fwts_cpu_readmsr(fw, i, 0x1a0, &val) != FWTS_OK) {
>  			fwts_log_error(fw, "Cannot read msr 0x1a0 on CPU%d", i);
>  			fwts_cpu_free_info(fwts_nx_cpuinfo);
>  			return FWTS_ERROR;
> diff --git a/src/cpu/virt/virt_svm.c b/src/cpu/virt/virt_svm.c
> index 8cf85305..59ebb83f 100644
> --- a/src/cpu/virt/virt_svm.c
> +++ b/src/cpu/virt/virt_svm.c
> @@ -52,14 +52,14 @@ static int can_lock_with_msr(void)
>  	return (fwts_virt_cpuinfo->x86 & 0x10);
>  }
>  
> -static int vt_locked_by_bios(void)
> +static int vt_locked_by_bios(fwts_framework *fw)
>  {
>  	uint64_t msr;
>  
>  	if (!can_lock_with_msr())
>  		return 0;
>  
> -	if (fwts_cpu_readmsr(0, MSR_FEATURE_CONTROL, &msr))
> +	if (fwts_cpu_readmsr(fw, 0, MSR_FEATURE_CONTROL, &msr))
>  		return -1;
>  
>  	return ((msr & 0x1000) == 0x1000); /* SVM capable but locked by bios*/
> @@ -72,7 +72,7 @@ void virt_check_svm(fwts_framework *fw)
>  	if (!cpu_has_svm())
>  		fwts_skipped(fw, "Processor does not support Virtualization extensions, won't test BIOS configuration, skipping test.");
>  	else  {
> -		int ret = vt_locked_by_bios();
> +		int ret = vt_locked_by_bios(fw);
>  		switch (ret) {
>  		case 0:
>  			fwts_passed(fw, "Virtualization extensions supported and enabled by BIOS.");
> diff --git a/src/cpu/virt/virt_vmx.c b/src/cpu/virt/virt_vmx.c
> index 307424e7..c4cfb6ab 100644
> --- a/src/cpu/virt/virt_vmx.c
> +++ b/src/cpu/virt/virt_vmx.c
> @@ -55,11 +55,11 @@ static int cpu_has_vmx(void)
>  	return (strstr(fwts_virt_cpuinfo->flags, "vmx") != NULL);
>  }
>  
> -static int vt_locked_by_bios(void)
> +static int vt_locked_by_bios(fwts_framework *fw)
>  {
>  	uint64_t msr;
>  
> -	if (fwts_cpu_readmsr(0, MSR_FEATURE_CONTROL, &msr))
> +	if (fwts_cpu_readmsr(fw, 0, MSR_FEATURE_CONTROL, &msr))
>  		return -1;
>  
>  	return (msr & 5) == 1; /* VT capable but locked by bios*/
> @@ -72,7 +72,7 @@ void virt_check_vmx(fwts_framework *fw)
>  	if (!cpu_has_vmx())
>  		fwts_skipped(fw, "Processor does not support Virtualization extensions, won't test BIOS configuration, skipping test.");
>  	else  {
> -		int ret = vt_locked_by_bios();
> +		int ret = vt_locked_by_bios(fw);
>  		switch (ret) {
>  		case 0:
>  			fwts_passed(fw, "Virtualization extensions supported and enabled by BIOS.");
> diff --git a/src/lib/include/fwts_cpu.h b/src/lib/include/fwts_cpu.h
> index 5aa92984..4192e571 100644
> --- a/src/lib/include/fwts_cpu.h
> +++ b/src/lib/include/fwts_cpu.h
> @@ -57,7 +57,7 @@ typedef struct cpu_benchmark_result {
>  	uint64_t	cycles;
>  } fwts_cpu_benchmark_result;
>  
> -int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val);
> +int fwts_cpu_readmsr(fwts_framework *fw, const int cpu, const uint32_t reg, uint64_t *val);
>  
>  int fwts_cpu_is_Intel(bool *is_intel);
>  int fwts_cpu_is_AMD(bool *is_amd);
> diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
> index 18ff7823..a3c1638a 100644
> --- a/src/lib/src/fwts_cpu.c
> +++ b/src/lib/src/fwts_cpu.c
> @@ -53,7 +53,11 @@ static pid_t *fwts_cpu_pids;
>   *  fwts_cpu_readmsr()
>   *	Read a given msr on a specificied CPU
>   */
> -int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val)
> +int fwts_cpu_readmsr(
> +	fwts_framework *fw,
> +	const int cpu,
> +	const uint32_t reg,
> +	uint64_t *val)
>  {
>  	char buffer[PATH_MAX];
>  	uint64_t value = 0;
> @@ -62,12 +66,18 @@ int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val)
>  
>  	snprintf(buffer, sizeof(buffer), "/dev/cpu/%d/msr", cpu);
>  	if ((fd = open(buffer, O_RDONLY)) < 0) {
> -		/* Hrm, msr not there, so force modprobe msr and see what happens */
> -		pid_t pid;
> -		if (fwts_pipe_open_ro("modprobe msr", &pid, &fd) < 0)
> -			return FWTS_ERROR;
> -		fwts_pipe_close(fd, pid);
> +		bool loaded = false;
>  
> +		/*
> +		 *  msr not there, so force a load of the msr
> +		 *  module and retry
> +		 */
> +		if (fwts_module_load(fw, "msr") != FWTS_OK)
> +			return FWTS_ERROR;
> +		if (fwts_module_loaded(fw, "msr", &loaded) != FWTS_OK)
> +			return FWTS_ERROR;
> +		if (!loaded)
> +			return FWTS_ERROR;
>  		if ((fd = open(buffer, O_RDONLY)) < 0)
>  			return FWTS_ERROR; /* Really failed */
>  	}
> 


Acked-by: Alex Hung <alex.hung@canonical.com>
ivanhu Nov. 13, 2018, 7:52 a.m. UTC | #2
On 11/8/18 10:45 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Make the msr library helper now use the new module loading
> functions. This involves adding an extra fw parameter which means
> msr releated tests need modifying too.
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  src/bios/mtrr/mtrr.c       |  7 ++++---
>  src/cpu/msr/msr.c          | 14 ++++++++------
>  src/cpu/nx/nx.c            |  2 +-
>  src/cpu/virt/virt_svm.c    |  6 +++---
>  src/cpu/virt/virt_vmx.c    |  6 +++---
>  src/lib/include/fwts_cpu.h |  2 +-
>  src/lib/src/fwts_cpu.c     | 22 ++++++++++++++++------
>  7 files changed, 36 insertions(+), 23 deletions(-)
>
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 5dfb4a60..4948b55a 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -182,8 +182,9 @@ static int get_mtrrs(void)
>  	return FWTS_OK;
>  }
>  
> -static int get_default_mtrr(void) {
> -	if (fwts_cpu_readmsr(0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
> +static int get_default_mtrr(fwts_framework *fw)
> +{
> +	if (fwts_cpu_readmsr(fw, 0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
>  		switch (mtrr_default & 0xFF) {
>  			case 0:
>  				mtrr_default = UNCACHED;
> @@ -408,7 +409,7 @@ static int validate_iomem(fwts_framework *fw)
>  	if ((file = fopen("/proc/iomem", "r")) == NULL)
>  		return FWTS_ERROR;
>  
> -	if (get_default_mtrr() != FWTS_OK)
> +	if (get_default_mtrr(fw) != FWTS_OK)
>  		mtrr_default = UNKNOWN;
>  
>  	while (!feof(file)) {
> diff --git a/src/cpu/msr/msr.c b/src/cpu/msr/msr.c
> index 70d89650..a9dfd40c 100644
> --- a/src/cpu/msr/msr.c
> +++ b/src/cpu/msr/msr.c
> @@ -77,7 +77,9 @@ static int msr_deinit(fwts_framework *fw)
>  	return FWTS_OK;
>  }
>  
> -static int msr_consistent(const uint32_t msr,
> +static int msr_consistent(
> +	fwts_framework *fw,
> +	const uint32_t msr,
>  	const int shift,
>  	const uint64_t mask,
>  	uint64_t *const vals,
> @@ -90,7 +92,7 @@ static int msr_consistent(const uint32_t msr,
>  
>  	for (cpu = 0; cpu < ncpus; cpu++) {
>  		uint64_t val;
> -		if (fwts_cpu_readmsr(cpu, msr, &val) != FWTS_OK) {
> +		if (fwts_cpu_readmsr(fw, cpu, msr, &val) != FWTS_OK) {
>  			return FWTS_ERROR;
>  		}
>  		val >>= shift;
> @@ -130,7 +132,7 @@ static int msr_consistent_check(fwts_framework *fw,
>  		free(vals);
>  		return FWTS_ERROR;
>  	}
> -	if (msr_consistent(msr, shift, mask,
> +	if (msr_consistent(fw, msr, shift, mask,
>  		vals, &inconsistent_count, inconsistent) != FWTS_OK) {
>  		free(inconsistent);
>  		free(vals);
> @@ -197,7 +199,7 @@ static int msr_smrr(fwts_framework *fw)
>  	uint64_t val;
>  
>  	if (intel_cpu) {
> -		if (fwts_cpu_readmsr(0, 0xfe, &val) != FWTS_OK) {
> +		if (fwts_cpu_readmsr(fw, 0, 0xfe, &val) != FWTS_OK) {
>  			fwts_skipped(fw, "Cannot read MSR 0xfe.");
>  			return FWTS_ERROR;
>  		}
> @@ -210,7 +212,7 @@ static int msr_smrr(fwts_framework *fw)
>  			msr_consistent_check(fw, LOG_LEVEL_HIGH, "SMRR_PHYSMASK", 0x1f3, 12, 0xfffff, NULL);
>  			msr_consistent_check(fw, LOG_LEVEL_HIGH, "SMRR_VALID", 0x1f3, 11, 0x1, NULL);
>  
> -			if (fwts_cpu_readmsr(0, 0x1f2, &val) == FWTS_OK) {
> +			if (fwts_cpu_readmsr(fw, 0, 0x1f2, &val) == FWTS_OK) {
>  				uint64_t physbase = val & 0xfffff000;
>  				uint64_t type = val & 7;
>  				if ((physbase & 0xfff) != 0)
> @@ -221,7 +223,7 @@ static int msr_smrr(fwts_framework *fw)
>  					fwts_failed(fw, LOG_LEVEL_HIGH, "MSRSMRR_TYPE",
>  						"SMRR: SMRR_TYPE is 0x%" PRIx64 ", should be 0x6 (Write-Back).", type);
>  			}
> -			if (fwts_cpu_readmsr(0, 0x1f3, &val) == FWTS_OK) {
> +			if (fwts_cpu_readmsr(fw, 0, 0x1f3, &val) == FWTS_OK) {
>  				uint64_t physmask = val & 0xfffff000;
>  				uint64_t valid = (val >> 11) & 1;
>  
> diff --git a/src/cpu/nx/nx.c b/src/cpu/nx/nx.c
> index 66a7b6f1..5caf0c6c 100644
> --- a/src/cpu/nx/nx.c
> +++ b/src/cpu/nx/nx.c
> @@ -169,7 +169,7 @@ static int nx_test3(fwts_framework *fw)
>  			fwts_cpu_free_info(fwts_nx_cpuinfo);
>  			return FWTS_OK;
>  		}
> -		if (fwts_cpu_readmsr(i, 0x1a0, &val) != FWTS_OK) {
> +		if (fwts_cpu_readmsr(fw, i, 0x1a0, &val) != FWTS_OK) {
>  			fwts_log_error(fw, "Cannot read msr 0x1a0 on CPU%d", i);
>  			fwts_cpu_free_info(fwts_nx_cpuinfo);
>  			return FWTS_ERROR;
> diff --git a/src/cpu/virt/virt_svm.c b/src/cpu/virt/virt_svm.c
> index 8cf85305..59ebb83f 100644
> --- a/src/cpu/virt/virt_svm.c
> +++ b/src/cpu/virt/virt_svm.c
> @@ -52,14 +52,14 @@ static int can_lock_with_msr(void)
>  	return (fwts_virt_cpuinfo->x86 & 0x10);
>  }
>  
> -static int vt_locked_by_bios(void)
> +static int vt_locked_by_bios(fwts_framework *fw)
>  {
>  	uint64_t msr;
>  
>  	if (!can_lock_with_msr())
>  		return 0;
>  
> -	if (fwts_cpu_readmsr(0, MSR_FEATURE_CONTROL, &msr))
> +	if (fwts_cpu_readmsr(fw, 0, MSR_FEATURE_CONTROL, &msr))
>  		return -1;
>  
>  	return ((msr & 0x1000) == 0x1000); /* SVM capable but locked by bios*/
> @@ -72,7 +72,7 @@ void virt_check_svm(fwts_framework *fw)
>  	if (!cpu_has_svm())
>  		fwts_skipped(fw, "Processor does not support Virtualization extensions, won't test BIOS configuration, skipping test.");
>  	else  {
> -		int ret = vt_locked_by_bios();
> +		int ret = vt_locked_by_bios(fw);
>  		switch (ret) {
>  		case 0:
>  			fwts_passed(fw, "Virtualization extensions supported and enabled by BIOS.");
> diff --git a/src/cpu/virt/virt_vmx.c b/src/cpu/virt/virt_vmx.c
> index 307424e7..c4cfb6ab 100644
> --- a/src/cpu/virt/virt_vmx.c
> +++ b/src/cpu/virt/virt_vmx.c
> @@ -55,11 +55,11 @@ static int cpu_has_vmx(void)
>  	return (strstr(fwts_virt_cpuinfo->flags, "vmx") != NULL);
>  }
>  
> -static int vt_locked_by_bios(void)
> +static int vt_locked_by_bios(fwts_framework *fw)
>  {
>  	uint64_t msr;
>  
> -	if (fwts_cpu_readmsr(0, MSR_FEATURE_CONTROL, &msr))
> +	if (fwts_cpu_readmsr(fw, 0, MSR_FEATURE_CONTROL, &msr))
>  		return -1;
>  
>  	return (msr & 5) == 1; /* VT capable but locked by bios*/
> @@ -72,7 +72,7 @@ void virt_check_vmx(fwts_framework *fw)
>  	if (!cpu_has_vmx())
>  		fwts_skipped(fw, "Processor does not support Virtualization extensions, won't test BIOS configuration, skipping test.");
>  	else  {
> -		int ret = vt_locked_by_bios();
> +		int ret = vt_locked_by_bios(fw);
>  		switch (ret) {
>  		case 0:
>  			fwts_passed(fw, "Virtualization extensions supported and enabled by BIOS.");
> diff --git a/src/lib/include/fwts_cpu.h b/src/lib/include/fwts_cpu.h
> index 5aa92984..4192e571 100644
> --- a/src/lib/include/fwts_cpu.h
> +++ b/src/lib/include/fwts_cpu.h
> @@ -57,7 +57,7 @@ typedef struct cpu_benchmark_result {
>  	uint64_t	cycles;
>  } fwts_cpu_benchmark_result;
>  
> -int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val);
> +int fwts_cpu_readmsr(fwts_framework *fw, const int cpu, const uint32_t reg, uint64_t *val);
>  
>  int fwts_cpu_is_Intel(bool *is_intel);
>  int fwts_cpu_is_AMD(bool *is_amd);
> diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
> index 18ff7823..a3c1638a 100644
> --- a/src/lib/src/fwts_cpu.c
> +++ b/src/lib/src/fwts_cpu.c
> @@ -53,7 +53,11 @@ static pid_t *fwts_cpu_pids;
>   *  fwts_cpu_readmsr()
>   *	Read a given msr on a specificied CPU
>   */
> -int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val)
> +int fwts_cpu_readmsr(
> +	fwts_framework *fw,
> +	const int cpu,
> +	const uint32_t reg,
> +	uint64_t *val)
>  {
>  	char buffer[PATH_MAX];
>  	uint64_t value = 0;
> @@ -62,12 +66,18 @@ int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val)
>  
>  	snprintf(buffer, sizeof(buffer), "/dev/cpu/%d/msr", cpu);
>  	if ((fd = open(buffer, O_RDONLY)) < 0) {
> -		/* Hrm, msr not there, so force modprobe msr and see what happens */
> -		pid_t pid;
> -		if (fwts_pipe_open_ro("modprobe msr", &pid, &fd) < 0)
> -			return FWTS_ERROR;
> -		fwts_pipe_close(fd, pid);
> +		bool loaded = false;
>  
> +		/*
> +		 *  msr not there, so force a load of the msr
> +		 *  module and retry
> +		 */
> +		if (fwts_module_load(fw, "msr") != FWTS_OK)
> +			return FWTS_ERROR;
> +		if (fwts_module_loaded(fw, "msr", &loaded) != FWTS_OK)
> +			return FWTS_ERROR;
> +		if (!loaded)
> +			return FWTS_ERROR;
>  		if ((fd = open(buffer, O_RDONLY)) < 0)
>  			return FWTS_ERROR; /* Really failed */
>  	}


Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch
diff mbox series

diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
index 5dfb4a60..4948b55a 100644
--- a/src/bios/mtrr/mtrr.c
+++ b/src/bios/mtrr/mtrr.c
@@ -182,8 +182,9 @@  static int get_mtrrs(void)
 	return FWTS_OK;
 }
 
-static int get_default_mtrr(void) {
-	if (fwts_cpu_readmsr(0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
+static int get_default_mtrr(fwts_framework *fw)
+{
+	if (fwts_cpu_readmsr(fw, 0, MTRR_DEF_TYPE_MSR, &mtrr_default) == FWTS_OK) {
 		switch (mtrr_default & 0xFF) {
 			case 0:
 				mtrr_default = UNCACHED;
@@ -408,7 +409,7 @@  static int validate_iomem(fwts_framework *fw)
 	if ((file = fopen("/proc/iomem", "r")) == NULL)
 		return FWTS_ERROR;
 
-	if (get_default_mtrr() != FWTS_OK)
+	if (get_default_mtrr(fw) != FWTS_OK)
 		mtrr_default = UNKNOWN;
 
 	while (!feof(file)) {
diff --git a/src/cpu/msr/msr.c b/src/cpu/msr/msr.c
index 70d89650..a9dfd40c 100644
--- a/src/cpu/msr/msr.c
+++ b/src/cpu/msr/msr.c
@@ -77,7 +77,9 @@  static int msr_deinit(fwts_framework *fw)
 	return FWTS_OK;
 }
 
-static int msr_consistent(const uint32_t msr,
+static int msr_consistent(
+	fwts_framework *fw,
+	const uint32_t msr,
 	const int shift,
 	const uint64_t mask,
 	uint64_t *const vals,
@@ -90,7 +92,7 @@  static int msr_consistent(const uint32_t msr,
 
 	for (cpu = 0; cpu < ncpus; cpu++) {
 		uint64_t val;
-		if (fwts_cpu_readmsr(cpu, msr, &val) != FWTS_OK) {
+		if (fwts_cpu_readmsr(fw, cpu, msr, &val) != FWTS_OK) {
 			return FWTS_ERROR;
 		}
 		val >>= shift;
@@ -130,7 +132,7 @@  static int msr_consistent_check(fwts_framework *fw,
 		free(vals);
 		return FWTS_ERROR;
 	}
-	if (msr_consistent(msr, shift, mask,
+	if (msr_consistent(fw, msr, shift, mask,
 		vals, &inconsistent_count, inconsistent) != FWTS_OK) {
 		free(inconsistent);
 		free(vals);
@@ -197,7 +199,7 @@  static int msr_smrr(fwts_framework *fw)
 	uint64_t val;
 
 	if (intel_cpu) {
-		if (fwts_cpu_readmsr(0, 0xfe, &val) != FWTS_OK) {
+		if (fwts_cpu_readmsr(fw, 0, 0xfe, &val) != FWTS_OK) {
 			fwts_skipped(fw, "Cannot read MSR 0xfe.");
 			return FWTS_ERROR;
 		}
@@ -210,7 +212,7 @@  static int msr_smrr(fwts_framework *fw)
 			msr_consistent_check(fw, LOG_LEVEL_HIGH, "SMRR_PHYSMASK", 0x1f3, 12, 0xfffff, NULL);
 			msr_consistent_check(fw, LOG_LEVEL_HIGH, "SMRR_VALID", 0x1f3, 11, 0x1, NULL);
 
-			if (fwts_cpu_readmsr(0, 0x1f2, &val) == FWTS_OK) {
+			if (fwts_cpu_readmsr(fw, 0, 0x1f2, &val) == FWTS_OK) {
 				uint64_t physbase = val & 0xfffff000;
 				uint64_t type = val & 7;
 				if ((physbase & 0xfff) != 0)
@@ -221,7 +223,7 @@  static int msr_smrr(fwts_framework *fw)
 					fwts_failed(fw, LOG_LEVEL_HIGH, "MSRSMRR_TYPE",
 						"SMRR: SMRR_TYPE is 0x%" PRIx64 ", should be 0x6 (Write-Back).", type);
 			}
-			if (fwts_cpu_readmsr(0, 0x1f3, &val) == FWTS_OK) {
+			if (fwts_cpu_readmsr(fw, 0, 0x1f3, &val) == FWTS_OK) {
 				uint64_t physmask = val & 0xfffff000;
 				uint64_t valid = (val >> 11) & 1;
 
diff --git a/src/cpu/nx/nx.c b/src/cpu/nx/nx.c
index 66a7b6f1..5caf0c6c 100644
--- a/src/cpu/nx/nx.c
+++ b/src/cpu/nx/nx.c
@@ -169,7 +169,7 @@  static int nx_test3(fwts_framework *fw)
 			fwts_cpu_free_info(fwts_nx_cpuinfo);
 			return FWTS_OK;
 		}
-		if (fwts_cpu_readmsr(i, 0x1a0, &val) != FWTS_OK) {
+		if (fwts_cpu_readmsr(fw, i, 0x1a0, &val) != FWTS_OK) {
 			fwts_log_error(fw, "Cannot read msr 0x1a0 on CPU%d", i);
 			fwts_cpu_free_info(fwts_nx_cpuinfo);
 			return FWTS_ERROR;
diff --git a/src/cpu/virt/virt_svm.c b/src/cpu/virt/virt_svm.c
index 8cf85305..59ebb83f 100644
--- a/src/cpu/virt/virt_svm.c
+++ b/src/cpu/virt/virt_svm.c
@@ -52,14 +52,14 @@  static int can_lock_with_msr(void)
 	return (fwts_virt_cpuinfo->x86 & 0x10);
 }
 
-static int vt_locked_by_bios(void)
+static int vt_locked_by_bios(fwts_framework *fw)
 {
 	uint64_t msr;
 
 	if (!can_lock_with_msr())
 		return 0;
 
-	if (fwts_cpu_readmsr(0, MSR_FEATURE_CONTROL, &msr))
+	if (fwts_cpu_readmsr(fw, 0, MSR_FEATURE_CONTROL, &msr))
 		return -1;
 
 	return ((msr & 0x1000) == 0x1000); /* SVM capable but locked by bios*/
@@ -72,7 +72,7 @@  void virt_check_svm(fwts_framework *fw)
 	if (!cpu_has_svm())
 		fwts_skipped(fw, "Processor does not support Virtualization extensions, won't test BIOS configuration, skipping test.");
 	else  {
-		int ret = vt_locked_by_bios();
+		int ret = vt_locked_by_bios(fw);
 		switch (ret) {
 		case 0:
 			fwts_passed(fw, "Virtualization extensions supported and enabled by BIOS.");
diff --git a/src/cpu/virt/virt_vmx.c b/src/cpu/virt/virt_vmx.c
index 307424e7..c4cfb6ab 100644
--- a/src/cpu/virt/virt_vmx.c
+++ b/src/cpu/virt/virt_vmx.c
@@ -55,11 +55,11 @@  static int cpu_has_vmx(void)
 	return (strstr(fwts_virt_cpuinfo->flags, "vmx") != NULL);
 }
 
-static int vt_locked_by_bios(void)
+static int vt_locked_by_bios(fwts_framework *fw)
 {
 	uint64_t msr;
 
-	if (fwts_cpu_readmsr(0, MSR_FEATURE_CONTROL, &msr))
+	if (fwts_cpu_readmsr(fw, 0, MSR_FEATURE_CONTROL, &msr))
 		return -1;
 
 	return (msr & 5) == 1; /* VT capable but locked by bios*/
@@ -72,7 +72,7 @@  void virt_check_vmx(fwts_framework *fw)
 	if (!cpu_has_vmx())
 		fwts_skipped(fw, "Processor does not support Virtualization extensions, won't test BIOS configuration, skipping test.");
 	else  {
-		int ret = vt_locked_by_bios();
+		int ret = vt_locked_by_bios(fw);
 		switch (ret) {
 		case 0:
 			fwts_passed(fw, "Virtualization extensions supported and enabled by BIOS.");
diff --git a/src/lib/include/fwts_cpu.h b/src/lib/include/fwts_cpu.h
index 5aa92984..4192e571 100644
--- a/src/lib/include/fwts_cpu.h
+++ b/src/lib/include/fwts_cpu.h
@@ -57,7 +57,7 @@  typedef struct cpu_benchmark_result {
 	uint64_t	cycles;
 } fwts_cpu_benchmark_result;
 
-int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val);
+int fwts_cpu_readmsr(fwts_framework *fw, const int cpu, const uint32_t reg, uint64_t *val);
 
 int fwts_cpu_is_Intel(bool *is_intel);
 int fwts_cpu_is_AMD(bool *is_amd);
diff --git a/src/lib/src/fwts_cpu.c b/src/lib/src/fwts_cpu.c
index 18ff7823..a3c1638a 100644
--- a/src/lib/src/fwts_cpu.c
+++ b/src/lib/src/fwts_cpu.c
@@ -53,7 +53,11 @@  static pid_t *fwts_cpu_pids;
  *  fwts_cpu_readmsr()
  *	Read a given msr on a specificied CPU
  */
-int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val)
+int fwts_cpu_readmsr(
+	fwts_framework *fw,
+	const int cpu,
+	const uint32_t reg,
+	uint64_t *val)
 {
 	char buffer[PATH_MAX];
 	uint64_t value = 0;
@@ -62,12 +66,18 @@  int fwts_cpu_readmsr(const int cpu, const uint32_t reg, uint64_t *val)
 
 	snprintf(buffer, sizeof(buffer), "/dev/cpu/%d/msr", cpu);
 	if ((fd = open(buffer, O_RDONLY)) < 0) {
-		/* Hrm, msr not there, so force modprobe msr and see what happens */
-		pid_t pid;
-		if (fwts_pipe_open_ro("modprobe msr", &pid, &fd) < 0)
-			return FWTS_ERROR;
-		fwts_pipe_close(fd, pid);
+		bool loaded = false;
 
+		/*
+		 *  msr not there, so force a load of the msr
+		 *  module and retry
+		 */
+		if (fwts_module_load(fw, "msr") != FWTS_OK)
+			return FWTS_ERROR;
+		if (fwts_module_loaded(fw, "msr", &loaded) != FWTS_OK)
+			return FWTS_ERROR;
+		if (!loaded)
+			return FWTS_ERROR;
 		if ((fd = open(buffer, O_RDONLY)) < 0)
 			return FWTS_ERROR; /* Really failed */
 	}