Patchwork [3/4,RESEND] bios: mtrr: don't use lspci to get PCI specific data (LP: #1244673)

login
register
mail settings
Submitter Colin King
Date Oct. 31, 2013, 10:33 a.m.
Message ID <1383215606-31054-1-git-send-email-colin.king@canonical.com>
Download mbox | patch
Permalink /patch/287454/
State Accepted
Headers show

Comments

Colin King - Oct. 31, 2013, 10:33 a.m.
From: Colin Ian King <colin.king@canonical.com>

Rather than do some messy parsing of the output from lspci, instead
just use the raw data from /sys/bus/pci/devices

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 src/bios/mtrr/mtrr.c | 85 ++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 59 insertions(+), 26 deletions(-)
Alex Hung - Nov. 6, 2013, 8:46 a.m.
On 10/31/2013 06:33 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Rather than do some messy parsing of the output from lspci, instead
> just use the raw data from /sys/bus/pci/devices
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/bios/mtrr/mtrr.c | 85 ++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 15ed37c..23afd41 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -201,35 +201,71 @@ static int check_prefetchable(
>   	uint64_t address,
>   	bool *pref)
>   {
> -	char line[4096];
> -	fwts_list *lspci_output;
> -	fwts_list_link *item;
> -	int status;
> +	char path[PATH_MAX];
> +	uint8_t config[64];
> +	int fd, i, n, bars;
> +	uint32_t *bar;
>
>   	*pref = false;
> +	snprintf(path, sizeof(path), FWTS_PCI_DEV_PATH "/%s/config", device);
> +	if ((fd = open(path, O_RDONLY)) < 0) {
> +		fwts_log_error(fw, "Cannot read PCI config for device %s\n", device);
> +		return FWTS_ERROR;
> +	}
>
> -	memset(line,0,4096);
> +	n = read(fd, config, sizeof(config));
> +	close(fd);
>
> -	snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
> -	if (fwts_pipe_exec(line, &lspci_output, &status) != FWTS_OK)
> -		return FWTS_ERROR;
> +	/* config space too small? ignore for now */
> +	if (n < 64)
> +		return FWTS_OK;
>
> -	/* Nothing for this decice? That seems like a failure */
> -	if (lspci_output == NULL)
> -		return FWTS_ERROR;
> +	/* Type, mask off top bit so we can cater for multi-function devices */
> +	switch (config[FWTS_PCI_CONFIG_HEADER_TYPE] & 0x7f) {
> +	case FWTS_PCI_CONFIG_HEADER_TYPE_NON_BRIDGE:
> +		bars = 6;
> +		break;
> +	case FWTS_PCI_CONFIG_HEADER_TYPE_PCI_BRIDGE:
> +		bars = 2;
> +		break;
> +	default:
> +		/* No BARs, ignore */
> +		return FWTS_OK;
> +	}
> +
> +	/*
> +	 *  Check BAR addresses, do they match and are they prefetchable
> +	 *  See http://wiki.osdev.org/PCI
> +	 */
> +	bar = (uint32_t*)&config[0x10];
> +	for (i = 0; i < bars; i++) {
> +		if ((bar[i] & 1) == 0) {
> +			uint64_t bar_addr;
> +			bool is_prefetchable =
> +				(bar[i] & FWTS_PCI_CONFIG_BAR_PREFETCHABLE);
> +
> +			switch ((bar[i] >> 1) & 0x03) {
> +			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_16BIT:
> +				bar_addr = bar[i] & 0xfff0;
> +				break;
> +			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_32BIT:
> +				bar_addr = bar[i] & 0xfffffff0;
> +				break;
> +			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_64BIT:
> +				bar_addr = (bar[i] & 0xfffffff0) |
> +					   (((uint64_t)bar[i+1]) << 32);
> +				i++;
> +				break;
> +			default:
> +				continue;
> +			}
>
> -	fwts_list_foreach(item, lspci_output) {
> -		char *str = strstr(fwts_text_list_text(item), "Memory at ");
> -		if (str && strtoull(str+10, NULL, 16) == address) {
> -			if (strstr(str, "non-prefetchable"))
> -				*pref = false;
> -			else if (strstr(str, "(prefetchable"))
> -				*pref = true;
> -			else if (strstr(str, ", prefetchable"))
> -				*pref = true;
> +			if (bar_addr == address) {
> +				*pref = is_prefetchable;
> +				break;
> +			}
>   		}
>   	}
> -	fwts_list_free(lspci_output, free);
>
>   	return FWTS_OK;
>   }
> @@ -334,8 +370,8 @@ static int validate_iomem(fwts_framework *fw)
>
>   		if (guess_cache_type(fw, c2, &type_must, &type_mustnot, start) != FWTS_OK) {
>   			/*  This has failed, give up at this point */
> -			fwts_skipped(fw,
> -				"Could not guess cache type, lspci seems to be failing.");
> +			fwts_skipped(fw,
> +				"Could not guess cache type.");
>   			fclose(file);
>   			return FWTS_ERROR;
>   		}
> @@ -404,9 +440,6 @@ static void do_mtrr_resource(fwts_framework *fw)
>
>   static int mtrr_init(fwts_framework *fw)
>   {
> -	if (fwts_check_executable(fw, fw->lspci, "lspci"))
> -		return FWTS_ERROR;
> -
>   	if (get_mtrrs() != FWTS_OK) {
>   		fwts_log_error(fw, "Failed to read /proc/mtrr.");
>   		return FWTS_ERROR;
>

Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu - Nov. 11, 2013, 9:52 a.m.
On 10/31/2013 06:33 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Rather than do some messy parsing of the output from lspci, instead
> just use the raw data from /sys/bus/pci/devices
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>   src/bios/mtrr/mtrr.c | 85 ++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
> index 15ed37c..23afd41 100644
> --- a/src/bios/mtrr/mtrr.c
> +++ b/src/bios/mtrr/mtrr.c
> @@ -201,35 +201,71 @@ static int check_prefetchable(
>   	uint64_t address,
>   	bool *pref)
>   {
> -	char line[4096];
> -	fwts_list *lspci_output;
> -	fwts_list_link *item;
> -	int status;
> +	char path[PATH_MAX];
> +	uint8_t config[64];
> +	int fd, i, n, bars;
> +	uint32_t *bar;
>
>   	*pref = false;
> +	snprintf(path, sizeof(path), FWTS_PCI_DEV_PATH "/%s/config", device);
> +	if ((fd = open(path, O_RDONLY)) < 0) {
> +		fwts_log_error(fw, "Cannot read PCI config for device %s\n", device);
> +		return FWTS_ERROR;
> +	}
>
> -	memset(line,0,4096);
> +	n = read(fd, config, sizeof(config));
> +	close(fd);
>
> -	snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
> -	if (fwts_pipe_exec(line, &lspci_output, &status) != FWTS_OK)
> -		return FWTS_ERROR;
> +	/* config space too small? ignore for now */
> +	if (n < 64)
> +		return FWTS_OK;
>
> -	/* Nothing for this decice? That seems like a failure */
> -	if (lspci_output == NULL)
> -		return FWTS_ERROR;
> +	/* Type, mask off top bit so we can cater for multi-function devices */
> +	switch (config[FWTS_PCI_CONFIG_HEADER_TYPE] & 0x7f) {
> +	case FWTS_PCI_CONFIG_HEADER_TYPE_NON_BRIDGE:
> +		bars = 6;
> +		break;
> +	case FWTS_PCI_CONFIG_HEADER_TYPE_PCI_BRIDGE:
> +		bars = 2;
> +		break;
> +	default:
> +		/* No BARs, ignore */
> +		return FWTS_OK;
> +	}
> +
> +	/*
> +	 *  Check BAR addresses, do they match and are they prefetchable
> +	 *  See http://wiki.osdev.org/PCI
> +	 */
> +	bar = (uint32_t*)&config[0x10];
> +	for (i = 0; i < bars; i++) {
> +		if ((bar[i] & 1) == 0) {
> +			uint64_t bar_addr;
> +			bool is_prefetchable =
> +				(bar[i] & FWTS_PCI_CONFIG_BAR_PREFETCHABLE);
> +
> +			switch ((bar[i] >> 1) & 0x03) {
> +			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_16BIT:
> +				bar_addr = bar[i] & 0xfff0;
> +				break;
> +			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_32BIT:
> +				bar_addr = bar[i] & 0xfffffff0;
> +				break;
> +			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_64BIT:
> +				bar_addr = (bar[i] & 0xfffffff0) |
> +					   (((uint64_t)bar[i+1]) << 32);
> +				i++;
> +				break;
> +			default:
> +				continue;
> +			}
>
> -	fwts_list_foreach(item, lspci_output) {
> -		char *str = strstr(fwts_text_list_text(item), "Memory at ");
> -		if (str && strtoull(str+10, NULL, 16) == address) {
> -			if (strstr(str, "non-prefetchable"))
> -				*pref = false;
> -			else if (strstr(str, "(prefetchable"))
> -				*pref = true;
> -			else if (strstr(str, ", prefetchable"))
> -				*pref = true;
> +			if (bar_addr == address) {
> +				*pref = is_prefetchable;
> +				break;
> +			}
>   		}
>   	}
> -	fwts_list_free(lspci_output, free);
>
>   	return FWTS_OK;
>   }
> @@ -334,8 +370,8 @@ static int validate_iomem(fwts_framework *fw)
>
>   		if (guess_cache_type(fw, c2, &type_must, &type_mustnot, start) != FWTS_OK) {
>   			/*  This has failed, give up at this point */
> -			fwts_skipped(fw,
> -				"Could not guess cache type, lspci seems to be failing.");
> +			fwts_skipped(fw,
> +				"Could not guess cache type.");
>   			fclose(file);
>   			return FWTS_ERROR;
>   		}
> @@ -404,9 +440,6 @@ static void do_mtrr_resource(fwts_framework *fw)
>
>   static int mtrr_init(fwts_framework *fw)
>   {
> -	if (fwts_check_executable(fw, fw->lspci, "lspci"))
> -		return FWTS_ERROR;
> -
>   	if (get_mtrrs() != FWTS_OK) {
>   		fwts_log_error(fw, "Failed to read /proc/mtrr.");
>   		return FWTS_ERROR;
>

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

Patch

diff --git a/src/bios/mtrr/mtrr.c b/src/bios/mtrr/mtrr.c
index 15ed37c..23afd41 100644
--- a/src/bios/mtrr/mtrr.c
+++ b/src/bios/mtrr/mtrr.c
@@ -201,35 +201,71 @@  static int check_prefetchable(
 	uint64_t address,
 	bool *pref)
 {
-	char line[4096];
-	fwts_list *lspci_output;
-	fwts_list_link *item;
-	int status;
+	char path[PATH_MAX];
+	uint8_t config[64];
+	int fd, i, n, bars;
+	uint32_t *bar;
 
 	*pref = false;
+	snprintf(path, sizeof(path), FWTS_PCI_DEV_PATH "/%s/config", device);
+	if ((fd = open(path, O_RDONLY)) < 0) {
+		fwts_log_error(fw, "Cannot read PCI config for device %s\n", device);
+		return FWTS_ERROR;
+	}
 
-	memset(line,0,4096);
+	n = read(fd, config, sizeof(config));
+	close(fd);
 
-	snprintf(line, sizeof(line), "%s -v -s %s", fw->lspci, device);
-	if (fwts_pipe_exec(line, &lspci_output, &status) != FWTS_OK)
-		return FWTS_ERROR;
+	/* config space too small? ignore for now */
+	if (n < 64)
+		return FWTS_OK;
 
-	/* Nothing for this decice? That seems like a failure */
-	if (lspci_output == NULL)
-		return FWTS_ERROR;
+	/* Type, mask off top bit so we can cater for multi-function devices */
+	switch (config[FWTS_PCI_CONFIG_HEADER_TYPE] & 0x7f) {
+	case FWTS_PCI_CONFIG_HEADER_TYPE_NON_BRIDGE:
+		bars = 6;
+		break;
+	case FWTS_PCI_CONFIG_HEADER_TYPE_PCI_BRIDGE:
+		bars = 2;
+		break;
+	default:
+		/* No BARs, ignore */
+		return FWTS_OK;
+	}
+
+	/*
+	 *  Check BAR addresses, do they match and are they prefetchable
+	 *  See http://wiki.osdev.org/PCI
+	 */
+	bar = (uint32_t*)&config[0x10];
+	for (i = 0; i < bars; i++) {
+		if ((bar[i] & 1) == 0) {
+			uint64_t bar_addr;
+			bool is_prefetchable =
+				(bar[i] & FWTS_PCI_CONFIG_BAR_PREFETCHABLE);
+
+			switch ((bar[i] >> 1) & 0x03) {
+			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_16BIT:
+				bar_addr = bar[i] & 0xfff0;
+				break;
+			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_32BIT:
+				bar_addr = bar[i] & 0xfffffff0;
+				break;
+			case FWTS_PCI_CONFIG_BAR_TYPE_ADDR_64BIT:
+				bar_addr = (bar[i] & 0xfffffff0) |
+					   (((uint64_t)bar[i+1]) << 32);
+				i++;
+				break;
+			default:
+				continue;
+			}
 
-	fwts_list_foreach(item, lspci_output) {
-		char *str = strstr(fwts_text_list_text(item), "Memory at ");
-		if (str && strtoull(str+10, NULL, 16) == address) {
-			if (strstr(str, "non-prefetchable"))
-				*pref = false;
-			else if (strstr(str, "(prefetchable"))
-				*pref = true;
-			else if (strstr(str, ", prefetchable"))
-				*pref = true;
+			if (bar_addr == address) {
+				*pref = is_prefetchable;
+				break;
+			}
 		}
 	}
-	fwts_list_free(lspci_output, free);
 
 	return FWTS_OK;
 }
@@ -334,8 +370,8 @@  static int validate_iomem(fwts_framework *fw)
 
 		if (guess_cache_type(fw, c2, &type_must, &type_mustnot, start) != FWTS_OK) {
 			/*  This has failed, give up at this point */
-			fwts_skipped(fw, 
-				"Could not guess cache type, lspci seems to be failing.");
+			fwts_skipped(fw,
+				"Could not guess cache type.");
 			fclose(file);
 			return FWTS_ERROR;
 		}
@@ -404,9 +440,6 @@  static void do_mtrr_resource(fwts_framework *fw)
 
 static int mtrr_init(fwts_framework *fw)
 {
-	if (fwts_check_executable(fw, fw->lspci, "lspci"))
-		return FWTS_ERROR;
-
 	if (get_mtrrs() != FWTS_OK) {
 		fwts_log_error(fw, "Failed to read /proc/mtrr.");
 		return FWTS_ERROR;