ACK: [PATCH 3/3] lib/fwts_cpu: use new use the new module loading helper functions

ivanhu ivan.hu at canonical.com
Tue Nov 13 07:52:46 UTC 2018


On 11/8/18 10:45 PM, Colin King wrote:
> From: Colin Ian King <colin.king at 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 at 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 at canonical.com>


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/fwts-devel/attachments/20181113/09df069d/attachment-0001.sig>


More information about the fwts-devel mailing list