[PATCH 00/10] disambiguate symbol names

[PATCH 00/10] disambiguate symbol names

[Jan Beulich]

Triggered by Konrad's needs for xSplice, but having been on my todo
list for a very long time to help interpretation of stack traces, here's a
first set of changes allowing to tell apart identically named static
symbols. The main and general thing is to prefix them by file name.
Some remaining collisions need other treatment: In some cases the
duplicates simply can be folded. In other cases renaming is necessary.
And then there are - compiler dependent - a few collisions left in place
by the end of this series. See the TBD in patch 10 for how to possibly
address their majority.

01: symbols: prefix static symbols with their source file name
02: x86/mm: override stored file names for multiply built sources
03: x86: don't build platform hypercall helpers multiple times
04: x86/HVM: prefix both instances of enable_intr_window()
05: x86/mm: build map_domain_gfn() just once
06: x86/mm: only a single instance of gw_page_flags[] is needed
07: x86/shadow: only a single instance of fetch_type_names[] is needed
08: x86/shadow: adjust sh_{make,destroy}_monitor_table() name tags
09: x86/shadow: drop stray name tags from sh_{guest_get,map}_eff_l1e()
10: [RFC] memory/compat: rename get_reserved_device_memory()

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 01/10] symbols: prefix static symbols with their source file name

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 8367 bytes --]

This requires adjustments to the tool generating the symbol table and
its as well as nm's invocation.

Note: Not warning about duplicate symbols in the EFI case for now, as
a binutils bug causes misnamed file name entries to appear in EFI
binaries' symbol tables when the file name is longer than 18 chars.
(Not doing so also avoids other duplicates getting printed twice.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
No changing ARM for now, as xSplice isn't targeted at it just yet.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -106,11 +106,13 @@ $(BASEDIR)/common/symbols-dummy.o:
 $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
-	$(NM) -n $(@D)/.$(@F).0 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0.S
+	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
+		| $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
-	$(NM) -n $(@D)/.$(@F).1 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1.S
+	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
+		| $(BASEDIR)/tools/symbols --sysv --sort --warn-dup >$(@D)/.$(@F).1.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(@D)/.$(@F).1.o -o $@
@@ -133,13 +135,15 @@ $(TARGET).efi: prelink-efi.o efi.lds efi
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
 	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
-	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).0 | $(guard) $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0s.S
+	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
+		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
-	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).1 | $(guard) $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1s.S
+	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
+		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
 	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o -o $@
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -30,6 +30,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <ctype.h>
 
 #define KSYM_NAME_LEN		127
@@ -40,13 +41,14 @@ struct sym_entry {
 	unsigned int len;
 	unsigned char *sym;
 };
-
+#define SYMBOL_NAME(s) ((char *)(s)->sym + 1)
 
 static struct sym_entry *table;
 static unsigned int table_size, table_cnt;
 static unsigned long long _stext, _etext, _sinittext, _einittext, _sextratext, _eextratext;
 static int all_symbols = 0;
 static char symbol_prefix_char = '\0';
+static enum { fmt_bsd, fmt_sysv } input_format;
 
 int token_profit[0x10000];
 
@@ -73,11 +75,25 @@ static inline int is_arm_mapping_symbol(
 
 static int read_symbol(FILE *in, struct sym_entry *s)
 {
-	char str[500];
+	char str[500], type[20] = "";
 	char *sym, stype;
-	int rc;
-
-	rc = fscanf(in, "%llx %c %499s\n", &s->addr, &stype, str);
+	static enum { symbol, source_file, object_file } last;
+	static char *filename;
+	int rc = -1;
+
+	switch (input_format) {
+	case fmt_bsd:
+		rc = fscanf(in, "%llx %c %499s\n", &s->addr, &stype, str);
+		break;
+	case fmt_sysv:
+		while (fscanf(in, "\n") == 1)
+			/* nothing */;
+		rc = fscanf(in, "%499[^ |] |%llx | %c |",
+			    str, &s->addr, &stype);
+		if (rc == 3 && fscanf(in, " %19[^ |] |", type) != 1)
+			*type = '\0';
+		break;
+	}
 	if (rc != 3) {
 		if (rc != EOF) {
 			/* skip line */
@@ -87,6 +103,28 @@ static int read_symbol(FILE *in, struct 
 		return -1;
 	}
 
+	sym = strrchr(str, '.');
+	if (strcasecmp(type, "FILE") == 0 ||
+	    (/* GNU nm prior to XXX doesn't produce a type for EFI binaries. */
+	     input_format == fmt_sysv && !*type && stype == '?' && sym &&
+	     sym[1] && strchr("cSsoh", sym[1]) && !sym[2])) {
+		/*
+		 * gas prior to XXX outputs symbol table entries resulting
+		 * from .file in reverse order. If we get two consecutive file
+		 * symbols, prefer the first one if that names an object file
+		 * (to cover multiply compiled files).
+		 */
+		if ((sym && sym[1] == 'o') || last != object_file) {
+			free(filename);
+			filename = *str ? strdup(str) : NULL;
+		}
+		last = !sym || sym[1] != 'o' ? source_file : object_file;
+		goto skip_tail;
+	}
+
+	last = symbol;
+	rc = -1;
+
 	sym = str;
 	/* skip prefix char */
 	if (symbol_prefix_char && str[0] == symbol_prefix_char)
@@ -109,24 +147,37 @@ static int read_symbol(FILE *in, struct 
 	{
 		/* Keep these useful absolute symbols */
 		if (strcmp(sym, "__gp"))
-			return -1;
-
+			goto skip_tail;
 	}
 	else if (toupper((uint8_t)stype) == 'U' ||
+		 toupper((uint8_t)stype) == 'N' ||
 		 is_arm_mapping_symbol(sym))
-		return -1;
+		goto skip_tail;
 	/* exclude also MIPS ELF local symbols ($L123 instead of .L123) */
 	else if (str[0] == '$')
-		return -1;
+		goto skip_tail;
 
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */
 	s->len = strlen(str) + 1;
+	if (islower(stype) && filename)
+		s->len += strlen(filename) + 1;
 	s->sym = malloc(s->len + 1);
-	strcpy((char *)s->sym + 1, str);
+	sym = SYMBOL_NAME(s);
+	if (islower(stype) && filename) {
+		sym = stpcpy(sym, filename);
+		*sym++ = '#';
+	}
+	strcpy(sym, str);
 	s->sym[0] = stype;
 
-	return 0;
+	rc = 0;
+
+ skip_tail:
+	if (input_format == fmt_sysv)
+		fgets(str, 500, in); /* discard rest of line */
+
+	return rc;
 }
 
 static int symbol_valid(struct sym_entry *s)
@@ -460,11 +512,37 @@ static void optimize_token_table(void)
 	optimize_result();
 }
 
+static int compare_value(const void *p1, const void *p2)
+{
+	const struct sym_entry *sym1 = p1;
+	const struct sym_entry *sym2 = p2;
+
+	if (sym1->addr < sym2->addr)
+		return -1;
+	if (sym1->addr > sym2->addr)
+		return +1;
+	/* Prefer global symbols. */
+	if (isupper(*sym1->sym))
+		return -1;
+	if (isupper(*sym2->sym))
+		return +1;
+	return 0;
+}
+
+static int compare_name(const void *p1, const void *p2)
+{
+	const struct sym_entry *sym1 = p1;
+	const struct sym_entry *sym2 = p2;
+
+	return strcmp(SYMBOL_NAME(sym1), SYMBOL_NAME(sym2));
+}
 
 int main(int argc, char **argv)
 {
+	unsigned int i;
+	bool unsorted = false, warn_dup = false;
+
 	if (argc >= 2) {
-		int i;
 		for (i = 1; i < argc; i++) {
 			if(strcmp(argv[i], "--all-symbols") == 0)
 				all_symbols = 1;
@@ -474,13 +552,36 @@ int main(int argc, char **argv)
 				if ((*p == '"' && *(p+2) == '"') || (*p == '\'' && *(p+2) == '\''))
 					p++;
 				symbol_prefix_char = *p;
-			} else
+			} else if (strcmp(argv[i], "--sysv") == 0)
+				input_format = fmt_sysv;
+			else if (strcmp(argv[i], "--sort") == 0)
+				unsorted = true;
+			else if (strcmp(argv[i], "--warn-dup") == 0)
+				warn_dup = true;
+			else
 				usage();
 		}
 	} else if (argc != 1)
 		usage();
 
 	read_map(stdin);
+
+	if (warn_dup) {
+		qsort(table, table_cnt, sizeof(*table), compare_name);
+		for (i = 1; i < table_cnt; ++i)
+			if (strcmp(SYMBOL_NAME(table + i - 1),
+				   SYMBOL_NAME(table + i)) == 0 &&
+			    table[i - 1].addr != table[i].addr)
+				fprintf(stderr,
+					"Duplicate symbol '%s' (%llx != %llx)\n",
+					SYMBOL_NAME(table + i),
+					table[i].addr, table[i - 1].addr);
+		unsorted = true;
+	}
+
+	if (unsorted)
+		qsort(table, table_cnt, sizeof(*table), compare_value);
+
 	optimize_token_table();
 	write_src();
 



[-- Attachment #2: static-syms-file.patch --]
[-- Type: text/plain, Size: 8425 bytes --]

symbols: prefix static symbols with their source file name

This requires adjustments to the tool generating the symbol table and
its as well as nm's invocation.

Note: Not warning about duplicate symbols in the EFI case for now, as
a binutils bug causes misnamed file name entries to appear in EFI
binaries' symbol tables when the file name is longer than 18 chars.
(Not doing so also avoids other duplicates getting printed twice.)

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
No changing ARM for now, as xSplice isn't targeted at it just yet.

--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -106,11 +106,13 @@ $(BASEDIR)/common/symbols-dummy.o:
 $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
-	$(NM) -n $(@D)/.$(@F).0 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0.S
+	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
+		| $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
-	$(NM) -n $(@D)/.$(@F).1 | $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1.S
+	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
+		| $(BASEDIR)/tools/symbols --sysv --sort --warn-dup >$(@D)/.$(@F).1.S
 	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
 	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
 	    $(@D)/.$(@F).1.o -o $@
@@ -133,13 +135,15 @@ $(TARGET).efi: prelink-efi.o efi.lds efi
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< efi/relocs-dummy.o \
 	                $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).$(base).0 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
-	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).0 | $(guard) $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0s.S
+	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
+		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).0s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o
 	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
 	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o $(@D)/.$(@F).$(base).1 &&) :
 	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE) $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
-	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).1 | $(guard) $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1s.S
+	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
+		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort >$(@D)/.$(@F).1s.S
 	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o
 	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $< \
 	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o -o $@
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -30,6 +30,7 @@
 #include <stdlib.h>
 #include <string.h>
 #include <stdint.h>
+#include <stdbool.h>
 #include <ctype.h>
 
 #define KSYM_NAME_LEN		127
@@ -40,13 +41,14 @@ struct sym_entry {
 	unsigned int len;
 	unsigned char *sym;
 };
-
+#define SYMBOL_NAME(s) ((char *)(s)->sym + 1)
 
 static struct sym_entry *table;
 static unsigned int table_size, table_cnt;
 static unsigned long long _stext, _etext, _sinittext, _einittext, _sextratext, _eextratext;
 static int all_symbols = 0;
 static char symbol_prefix_char = '\0';
+static enum { fmt_bsd, fmt_sysv } input_format;
 
 int token_profit[0x10000];
 
@@ -73,11 +75,25 @@ static inline int is_arm_mapping_symbol(
 
 static int read_symbol(FILE *in, struct sym_entry *s)
 {
-	char str[500];
+	char str[500], type[20] = "";
 	char *sym, stype;
-	int rc;
-
-	rc = fscanf(in, "%llx %c %499s\n", &s->addr, &stype, str);
+	static enum { symbol, source_file, object_file } last;
+	static char *filename;
+	int rc = -1;
+
+	switch (input_format) {
+	case fmt_bsd:
+		rc = fscanf(in, "%llx %c %499s\n", &s->addr, &stype, str);
+		break;
+	case fmt_sysv:
+		while (fscanf(in, "\n") == 1)
+			/* nothing */;
+		rc = fscanf(in, "%499[^ |] |%llx | %c |",
+			    str, &s->addr, &stype);
+		if (rc == 3 && fscanf(in, " %19[^ |] |", type) != 1)
+			*type = '\0';
+		break;
+	}
 	if (rc != 3) {
 		if (rc != EOF) {
 			/* skip line */
@@ -87,6 +103,28 @@ static int read_symbol(FILE *in, struct 
 		return -1;
 	}
 
+	sym = strrchr(str, '.');
+	if (strcasecmp(type, "FILE") == 0 ||
+	    (/* GNU nm prior to XXX doesn't produce a type for EFI binaries. */
+	     input_format == fmt_sysv && !*type && stype == '?' && sym &&
+	     sym[1] && strchr("cSsoh", sym[1]) && !sym[2])) {
+		/*
+		 * gas prior to XXX outputs symbol table entries resulting
+		 * from .file in reverse order. If we get two consecutive file
+		 * symbols, prefer the first one if that names an object file
+		 * (to cover multiply compiled files).
+		 */
+		if ((sym && sym[1] == 'o') || last != object_file) {
+			free(filename);
+			filename = *str ? strdup(str) : NULL;
+		}
+		last = !sym || sym[1] != 'o' ? source_file : object_file;
+		goto skip_tail;
+	}
+
+	last = symbol;
+	rc = -1;
+
 	sym = str;
 	/* skip prefix char */
 	if (symbol_prefix_char && str[0] == symbol_prefix_char)
@@ -109,24 +147,37 @@ static int read_symbol(FILE *in, struct 
 	{
 		/* Keep these useful absolute symbols */
 		if (strcmp(sym, "__gp"))
-			return -1;
-
+			goto skip_tail;
 	}
 	else if (toupper((uint8_t)stype) == 'U' ||
+		 toupper((uint8_t)stype) == 'N' ||
 		 is_arm_mapping_symbol(sym))
-		return -1;
+		goto skip_tail;
 	/* exclude also MIPS ELF local symbols ($L123 instead of .L123) */
 	else if (str[0] == '$')
-		return -1;
+		goto skip_tail;
 
 	/* include the type field in the symbol name, so that it gets
 	 * compressed together */
 	s->len = strlen(str) + 1;
+	if (islower(stype) && filename)
+		s->len += strlen(filename) + 1;
 	s->sym = malloc(s->len + 1);
-	strcpy((char *)s->sym + 1, str);
+	sym = SYMBOL_NAME(s);
+	if (islower(stype) && filename) {
+		sym = stpcpy(sym, filename);
+		*sym++ = '#';
+	}
+	strcpy(sym, str);
 	s->sym[0] = stype;
 
-	return 0;
+	rc = 0;
+
+ skip_tail:
+	if (input_format == fmt_sysv)
+		fgets(str, 500, in); /* discard rest of line */
+
+	return rc;
 }
 
 static int symbol_valid(struct sym_entry *s)
@@ -460,11 +512,37 @@ static void optimize_token_table(void)
 	optimize_result();
 }
 
+static int compare_value(const void *p1, const void *p2)
+{
+	const struct sym_entry *sym1 = p1;
+	const struct sym_entry *sym2 = p2;
+
+	if (sym1->addr < sym2->addr)
+		return -1;
+	if (sym1->addr > sym2->addr)
+		return +1;
+	/* Prefer global symbols. */
+	if (isupper(*sym1->sym))
+		return -1;
+	if (isupper(*sym2->sym))
+		return +1;
+	return 0;
+}
+
+static int compare_name(const void *p1, const void *p2)
+{
+	const struct sym_entry *sym1 = p1;
+	const struct sym_entry *sym2 = p2;
+
+	return strcmp(SYMBOL_NAME(sym1), SYMBOL_NAME(sym2));
+}
 
 int main(int argc, char **argv)
 {
+	unsigned int i;
+	bool unsorted = false, warn_dup = false;
+
 	if (argc >= 2) {
-		int i;
 		for (i = 1; i < argc; i++) {
 			if(strcmp(argv[i], "--all-symbols") == 0)
 				all_symbols = 1;
@@ -474,13 +552,36 @@ int main(int argc, char **argv)
 				if ((*p == '"' && *(p+2) == '"') || (*p == '\'' && *(p+2) == '\''))
 					p++;
 				symbol_prefix_char = *p;
-			} else
+			} else if (strcmp(argv[i], "--sysv") == 0)
+				input_format = fmt_sysv;
+			else if (strcmp(argv[i], "--sort") == 0)
+				unsorted = true;
+			else if (strcmp(argv[i], "--warn-dup") == 0)
+				warn_dup = true;
+			else
 				usage();
 		}
 	} else if (argc != 1)
 		usage();
 
 	read_map(stdin);
+
+	if (warn_dup) {
+		qsort(table, table_cnt, sizeof(*table), compare_name);
+		for (i = 1; i < table_cnt; ++i)
+			if (strcmp(SYMBOL_NAME(table + i - 1),
+				   SYMBOL_NAME(table + i)) == 0 &&
+			    table[i - 1].addr != table[i].addr)
+				fprintf(stderr,
+					"Duplicate symbol '%s' (%llx != %llx)\n",
+					SYMBOL_NAME(table + i),
+					table[i].addr, table[i - 1].addr);
+		unsorted = true;
+	}
+
+	if (unsorted)
+		qsort(table, table_cnt, sizeof(*table), compare_value);
+
 	optimize_token_table();
 	write_src();
 

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 01/10] symbols: prefix static symbols with their source file name

[Ian Campbell]

On Tue, 2015-10-20 at 04:38 -0600, Jan Beulich wrote:
> This requires adjustments to the tool generating the symbol table and
> its as well as nm's invocation.
> 
> Note: Not warning about duplicate symbols in the EFI case for now, as
> a binutils bug causes misnamed file name entries to appear in EFI
> binaries' symbol tables when the file name is longer than 18 chars.
> (Not doing so also avoids other duplicates getting printed twice.)
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> No changing ARM for now, as xSplice isn't targeted at it just yet.

The improved information from e.g. stack traces (which I'm supposing this
also enables) seems valuable on ARM too. Is this something you plan to add
or shall we ARM folks add it to our todo?

I suspect the answer is no, but do you think there is much scope for making
some of these complex linker runes common instead of arch specific?

>From what I can tell the actual arch change here is s/-n/-pa/ on the NM
invocation (numeric sort => no sort + include debugger symbols) and then
add --sysv --sort and optionally --want-dup to the tools/symbols
invocation. Is it really as simple as that?

Given we don't do a special EFI build/link I think on ARM we do want --warn
-dup if at all possible.

Ian.

> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -106,11 +106,13 @@ $(BASEDIR)/common/symbols-dummy.o:
>  $(TARGET)-syms: prelink.o xen.lds $(BASEDIR)/common/symbols-dummy.o
>  	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>  	    $(BASEDIR)/common/symbols-dummy.o -o $(@D)/.$(@F).0
> -	$(NM) -n $(@D)/.$(@F).0 | $(BASEDIR)/tools/symbols
> >$(@D)/.$(@F).0.S
> +	$(NM) -pa --format=sysv $(@D)/.$(@F).0 \
> +		| $(BASEDIR)/tools/symbols --sysv --sort
> >$(@D)/.$(@F).0.S
>  	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0.o
>  	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>  	    $(@D)/.$(@F).0.o -o $(@D)/.$(@F).1
> -	$(NM) -n $(@D)/.$(@F).1 | $(BASEDIR)/tools/symbols
> >$(@D)/.$(@F).1.S
> +	$(NM) -pa --format=sysv $(@D)/.$(@F).1 \
> +		| $(BASEDIR)/tools/symbols --sysv --sort --warn-dup
> >$(@D)/.$(@F).1.S
>  	$(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1.o
>  	$(LD) $(LDFLAGS) -T xen.lds -N prelink.o \
>  	    $(@D)/.$(@F).1.o -o $@
> @@ -133,13 +135,15 @@ $(TARGET).efi: prelink-efi.o efi.lds efi
>  	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds 
> -N $< efi/relocs-dummy.o \
>  	                $(BASEDIR)/common/symbols-dummy.o -o
> $(@D)/.$(@F).$(base).0 &&) :
>  	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE)
> $(ALT_BASE),$(@D)/.$(@F).$(base).0) >$(@D)/.$(@F).0r.S
> -	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).0 | $(guard)
> $(BASEDIR)/tools/symbols >$(@D)/.$(@F).0s.S
> +	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).0 \
> +		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort
> >$(@D)/.$(@F).0s.S
>  	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).0r.o
> $(@D)/.$(@F).0s.o
>  	$(foreach base, $(VIRT_BASE) $(ALT_BASE), \
>  	          $(guard) $(LD) $(call EFI_LDFLAGS,$(base)) -T efi.lds 
> -N $< \
>  	                $(@D)/.$(@F).0r.o $(@D)/.$(@F).0s.o -o
> $(@D)/.$(@F).$(base).1 &&) :
>  	$(guard) efi/mkreloc $(foreach base,$(VIRT_BASE)
> $(ALT_BASE),$(@D)/.$(@F).$(base).1) >$(@D)/.$(@F).1r.S
> -	$(guard) $(NM) -n $(@D)/.$(@F).$(VIRT_BASE).1 | $(guard)
> $(BASEDIR)/tools/symbols >$(@D)/.$(@F).1s.S
> +	$(guard) $(NM) -pa --format=sysv $(@D)/.$(@F).$(VIRT_BASE).1 \
> +		| $(guard) $(BASEDIR)/tools/symbols --sysv --sort
> >$(@D)/.$(@F).1s.S
>  	$(guard) $(MAKE) -f $(BASEDIR)/Rules.mk $(@D)/.$(@F).1r.o
> $(@D)/.$(@F).1s.o
>  	$(guard) $(LD) $(call EFI_LDFLAGS,$(VIRT_BASE)) -T efi.lds -N $<
> \
>  	                $(@D)/.$(@F).1r.o $(@D)/.$(@F).1s.o -o $@
> --- a/xen/tools/symbols.c
> +++ b/xen/tools/symbols.c
> @@ -30,6 +30,7 @@
>  #include <stdlib.h>
>  #include <string.h>
>  #include <stdint.h>
> +#include <stdbool.h>
>  #include <ctype.h>
>  
>  #define KSYM_NAME_LEN		127
> @@ -40,13 +41,14 @@ struct sym_entry {
>  	unsigned int len;
>  	unsigned char *sym;
>  };
> -
> +#define SYMBOL_NAME(s) ((char *)(s)->sym + 1)
>  
>  static struct sym_entry *table;
>  static unsigned int table_size, table_cnt;
>  static unsigned long long _stext, _etext, _sinittext, _einittext,
> _sextratext, _eextratext;
>  static int all_symbols = 0;
>  static char symbol_prefix_char = '\0';
> +static enum { fmt_bsd, fmt_sysv } input_format;
>  
>  int token_profit[0x10000];
>  
> @@ -73,11 +75,25 @@ static inline int is_arm_mapping_symbol(
>  
>  static int read_symbol(FILE *in, struct sym_entry *s)
>  {
> -	char str[500];
> +	char str[500], type[20] = "";
>  	char *sym, stype;
> -	int rc;
> -
> -	rc = fscanf(in, "%llx %c %499s\n", &s->addr, &stype, str);
> +	static enum { symbol, source_file, object_file } last;
> +	static char *filename;
> +	int rc = -1;
> +
> +	switch (input_format) {
> +	case fmt_bsd:
> +		rc = fscanf(in, "%llx %c %499s\n", &s->addr, &stype,
> str);
> +		break;
> +	case fmt_sysv:
> +		while (fscanf(in, "\n") == 1)
> +			/* nothing */;
> +		rc = fscanf(in, "%499[^ |] |%llx | %c |",
> +			    str, &s->addr, &stype);
> +		if (rc == 3 && fscanf(in, " %19[^ |] |", type) != 1)
> +			*type = '\0';
> +		break;
> +	}
>  	if (rc != 3) {
>  		if (rc != EOF) {
>  			/* skip line */
> @@ -87,6 +103,28 @@ static int read_symbol(FILE *in, struct 
>  		return -1;
>  	}
>  
> +	sym = strrchr(str, '.');
> +	if (strcasecmp(type, "FILE") == 0 ||
> +	    (/* GNU nm prior to XXX doesn't produce a type for EFI
> binaries. */
> +	     input_format == fmt_sysv && !*type && stype == '?' && sym
> &&
> +	     sym[1] && strchr("cSsoh", sym[1]) && !sym[2])) {
> +		/*
> +		 * gas prior to XXX outputs symbol table entries
> resulting
> +		 * from .file in reverse order. If we get two
> consecutive file
> +		 * symbols, prefer the first one if that names an object
> file
> +		 * (to cover multiply compiled files).
> +		 */
> +		if ((sym && sym[1] == 'o') || last != object_file) {
> +			free(filename);
> +			filename = *str ? strdup(str) : NULL;
> +		}
> +		last = !sym || sym[1] != 'o' ? source_file :
> object_file;
> +		goto skip_tail;
> +	}
> +
> +	last = symbol;
> +	rc = -1;
> +
>  	sym = str;
>  	/* skip prefix char */
>  	if (symbol_prefix_char && str[0] == symbol_prefix_char)
> @@ -109,24 +147,37 @@ static int read_symbol(FILE *in, struct 
>  	{
>  		/* Keep these useful absolute symbols */
>  		if (strcmp(sym, "__gp"))
> -			return -1;
> -
> +			goto skip_tail;
>  	}
>  	else if (toupper((uint8_t)stype) == 'U' ||
> +		 toupper((uint8_t)stype) == 'N' ||
>  		 is_arm_mapping_symbol(sym))
> -		return -1;
> +		goto skip_tail;
>  	/* exclude also MIPS ELF local symbols ($L123 instead of .L123)
> */
>  	else if (str[0] == '$')
> -		return -1;
> +		goto skip_tail;
>  
>  	/* include the type field in the symbol name, so that it gets
>  	 * compressed together */
>  	s->len = strlen(str) + 1;
> +	if (islower(stype) && filename)
> +		s->len += strlen(filename) + 1;
>  	s->sym = malloc(s->len + 1);
> -	strcpy((char *)s->sym + 1, str);
> +	sym = SYMBOL_NAME(s);
> +	if (islower(stype) && filename) {
> +		sym = stpcpy(sym, filename);
> +		*sym++ = '#';
> +	}
> +	strcpy(sym, str);
>  	s->sym[0] = stype;
>  
> -	return 0;
> +	rc = 0;
> +
> + skip_tail:
> +	if (input_format == fmt_sysv)
> +		fgets(str, 500, in); /* discard rest of line */
> +
> +	return rc;
>  }
>  
>  static int symbol_valid(struct sym_entry *s)
> @@ -460,11 +512,37 @@ static void optimize_token_table(void)
>  	optimize_result();
>  }
>  
> +static int compare_value(const void *p1, const void *p2)
> +{
> +	const struct sym_entry *sym1 = p1;
> +	const struct sym_entry *sym2 = p2;
> +
> +	if (sym1->addr < sym2->addr)
> +		return -1;
> +	if (sym1->addr > sym2->addr)
> +		return +1;
> +	/* Prefer global symbols. */
> +	if (isupper(*sym1->sym))
> +		return -1;
> +	if (isupper(*sym2->sym))
> +		return +1;
> +	return 0;
> +}
> +
> +static int compare_name(const void *p1, const void *p2)
> +{
> +	const struct sym_entry *sym1 = p1;
> +	const struct sym_entry *sym2 = p2;
> +
> +	return strcmp(SYMBOL_NAME(sym1), SYMBOL_NAME(sym2));
> +}
>  
>  int main(int argc, char **argv)
>  {
> +	unsigned int i;
> +	bool unsorted = false, warn_dup = false;
> +
>  	if (argc >= 2) {
> -		int i;
>  		for (i = 1; i < argc; i++) {
>  			if(strcmp(argv[i], "--all-symbols") == 0)
>  				all_symbols = 1;
> @@ -474,13 +552,36 @@ int main(int argc, char **argv)
>  				if ((*p == '"' && *(p+2) == '"') || (*p
> == '\'' && *(p+2) == '\''))
>  					p++;
>  				symbol_prefix_char = *p;
> -			} else
> +			} else if (strcmp(argv[i], "--sysv") == 0)
> +				input_format = fmt_sysv;
> +			else if (strcmp(argv[i], "--sort") == 0)
> +				unsorted = true;
> +			else if (strcmp(argv[i], "--warn-dup") == 0)
> +				warn_dup = true;
> +			else
>  				usage();
>  		}
>  	} else if (argc != 1)
>  		usage();
>  
>  	read_map(stdin);
> +
> +	if (warn_dup) {
> +		qsort(table, table_cnt, sizeof(*table), compare_name);
> +		for (i = 1; i < table_cnt; ++i)
> +			if (strcmp(SYMBOL_NAME(table + i - 1),
> +				   SYMBOL_NAME(table + i)) == 0 &&
> +			    table[i - 1].addr != table[i].addr)
> +				fprintf(stderr,
> +					"Duplicate symbol '%s' (%llx !=
> %llx)\n",
> +					SYMBOL_NAME(table + i),
> +					table[i].addr, table[i -
> 1].addr);
> +		unsorted = true;
> +	}
> +
> +	if (unsorted)
> +		qsort(table, table_cnt, sizeof(*table), compare_value);
> +
>  	optimize_token_table();
>  	write_src();
>  
> 
> 

Re: [PATCH 01/10] symbols: prefix static symbols with their source file name

[Jan Beulich]

>>> On 21.10.15 at 10:43, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-10-20 at 04:38 -0600, Jan Beulich wrote:
>> This requires adjustments to the tool generating the symbol table and
>> its as well as nm's invocation.
>> 
>> Note: Not warning about duplicate symbols in the EFI case for now, as
>> a binutils bug causes misnamed file name entries to appear in EFI
>> binaries' symbol tables when the file name is longer than 18 chars.
>> (Not doing so also avoids other duplicates getting printed twice.)
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> No changing ARM for now, as xSplice isn't targeted at it just yet.
> 
> The improved information from e.g. stack traces (which I'm supposing this
> also enables) seems valuable on ARM too. Is this something you plan to add
> or shall we ARM folks add it to our todo?

I can certainly implement it, but I can't - other than for x86 - verify
(other than by looking at the binary) that it has the intended effect.
(Of course all that hoping that I won't find more binutils issues in
the course of doing so.)

> I suspect the answer is no, but do you think there is much scope for making
> some of these complex linker runes common instead of arch specific?

I had considered this a while ago, but deemed it not worth it due
to the xen.efi rule necessarily being arch-specific (at least it would
be quite a bit harder to abstract this). But I guess we could move it
to xen/Rules.mk

> From what I can tell the actual arch change here is s/-n/-pa/ on the NM
> invocation (numeric sort => no sort + include debugger symbols) and then
> add --sysv --sort and optionally --want-dup to the tools/symbols
> invocation. Is it really as simple as that?

It is, luckily.

> Given we don't do a special EFI build/link I think on ARM we do want --warn
> -dup if at all possible.

Ultimately this is mostly useful for xSplice; when just considering stack
traces, a couple of duplicates don't really do much harm. But of course
using the same options for both architectures would simplify the making
common of the rule.

Jan

[PATCH 02/10] x86/mm: override stored file names for multiply built sources

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

To make it possible to tell apart the static symbols therein, use their
object file names instead of their source ones.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -29,6 +29,8 @@
 #include <asm/page.h>
 #include <asm/guest_pt.h>
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
 
 /* Flags that are needed in a pagetable entry, with the sense of NX inverted */
 static uint32_t mandatory_flags(struct vcpu *v, uint32_t pfec) 
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -38,6 +38,9 @@
 #include <asm/guest_pt.h>
 #include <asm/p2m.h>
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) "level.o\"");
+
 unsigned long hap_gva_to_gfn(GUEST_PAGING_LEVELS)(
     struct vcpu *v, struct p2m_domain *p2m, unsigned long gva, uint32_t *pfec)
 {
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -41,6 +41,9 @@
 #include "private.h"
 #include "types.h"
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"guest_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
+
 /* THINGS TO DO LATER:
  *
  * TEARDOWN HEURISTICS




[-- Attachment #2: x86-mm-rename-multiply-built.patch --]
[-- Type: text/plain, Size: 1474 bytes --]

x86/mm: override stored file names for multiply built sources

To make it possible to tell apart the static symbols therein, use their
object file names instead of their source ones.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -29,6 +29,8 @@
 #include <asm/page.h>
 #include <asm/guest_pt.h>
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
 
 /* Flags that are needed in a pagetable entry, with the sense of NX inverted */
 static uint32_t mandatory_flags(struct vcpu *v, uint32_t pfec) 
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -38,6 +38,9 @@
 #include <asm/guest_pt.h>
 #include <asm/p2m.h>
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) "level.o\"");
+
 unsigned long hap_gva_to_gfn(GUEST_PAGING_LEVELS)(
     struct vcpu *v, struct p2m_domain *p2m, unsigned long gva, uint32_t *pfec)
 {
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -41,6 +41,9 @@
 #include "private.h"
 #include "types.h"
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"guest_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
+
 /* THINGS TO DO LATER:
  *
  * TEARDOWN HEURISTICS

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 02/10] x86/mm: override stored file names for multiply built sources

[George Dunlap]

On 20/10/15 11:39, Jan Beulich wrote:
> To make it possible to tell apart the static symbols therein, use their
> object file names instead of their source ones.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I have no idea if the runes are correct, but the idea sounds great:

Acked-by: George Dunlap <george.dunlap@citrix.com>

> 
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -29,6 +29,8 @@
>  #include <asm/page.h>
>  #include <asm/guest_pt.h>
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
>  
>  /* Flags that are needed in a pagetable entry, with the sense of NX inverted */
>  static uint32_t mandatory_flags(struct vcpu *v, uint32_t pfec) 
> --- a/xen/arch/x86/mm/hap/guest_walk.c
> +++ b/xen/arch/x86/mm/hap/guest_walk.c
> @@ -38,6 +38,9 @@
>  #include <asm/guest_pt.h>
>  #include <asm/p2m.h>
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) "level.o\"");
> +
>  unsigned long hap_gva_to_gfn(GUEST_PAGING_LEVELS)(
>      struct vcpu *v, struct p2m_domain *p2m, unsigned long gva, uint32_t *pfec)
>  {
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -41,6 +41,9 @@
>  #include "private.h"
>  #include "types.h"
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"guest_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
> +
>  /* THINGS TO DO LATER:
>   *
>   * TEARDOWN HEURISTICS
> 
> 
> 

Re: [PATCH 02/10] x86/mm: override stored file names for multiply built sources

[Tim Deegan]

At 04:39 -0600 on 20 Oct (1445315964), Jan Beulich wrote:
> To make it possible to tell apart the static symbols therein, use their
> object file names instead of their source ones.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

Re: [PATCH 02/10] x86/mm: override stored file names for multiply built sources

[Andrew Cooper]

On 20/10/15 11:39, Jan Beulich wrote:
> To make it possible to tell apart the static symbols therein, use their
> object file names instead of their source ones.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

In principle, a very good idea.

Is it perhaps worth having the build runes pass in a -D value instead,
so as to avoid the .file reference getting stale?

>
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -29,6 +29,8 @@
>  #include <asm/page.h>
>  #include <asm/guest_pt.h>
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");

You should probably have a semicolon or newline at the end, to avoid
interacting with whatever the next directive the compiler chooses to
emit is.

~Andrew

Re: [PATCH 02/10] x86/mm: override stored file names for multiply built sources

[Jan Beulich]

>>> On 20.10.15 at 14:44, <andrew.cooper3@citrix.com> wrote:
> On 20/10/15 11:39, Jan Beulich wrote:
>> To make it possible to tell apart the static symbols therein, use their
>> object file names instead of their source ones.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> In principle, a very good idea.
> 
> Is it perhaps worth having the build runes pass in a -D value instead,
> so as to avoid the .file reference getting stale?

They're not required to be in sync.

>> --- a/xen/arch/x86/mm/guest_walk.c
>> +++ b/xen/arch/x86/mm/guest_walk.c
>> @@ -29,6 +29,8 @@
>>  #include <asm/page.h>
>>  #include <asm/guest_pt.h>
>>  
>> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
>> +asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
> 
> You should probably have a semicolon or newline at the end, to avoid
> interacting with whatever the next directive the compiler chooses to
> emit is.

We don't do this elsewhere (and the compiler takes care of this
anyway), so why should we care to do so here?

Jan

Re: [PATCH 02/10] x86/mm: override stored file names for multiply built sources

[Andrew Cooper]

On 20/10/15 14:12, Jan Beulich wrote:
>>>> On 20.10.15 at 14:44, <andrew.cooper3@citrix.com> wrote:
>> On 20/10/15 11:39, Jan Beulich wrote:
>>> To make it possible to tell apart the static symbols therein, use their
>>> object file names instead of their source ones.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> In principle, a very good idea.
>>
>> Is it perhaps worth having the build runes pass in a -D value instead,
>> so as to avoid the .file reference getting stale?
> They're not required to be in sync.

Of course they are not required, but it will be incredibly confusing to
debug if they are out of sync.

>
>>> --- a/xen/arch/x86/mm/guest_walk.c
>>> +++ b/xen/arch/x86/mm/guest_walk.c
>>> @@ -29,6 +29,8 @@
>>>  #include <asm/page.h>
>>>  #include <asm/guest_pt.h>
>>>  
>>> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
>>> +asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
>> You should probably have a semicolon or newline at the end, to avoid
>> interacting with whatever the next directive the compiler chooses to
>> emit is.
> We don't do this elsewhere (and the compiler takes care of this
> anyway), so why should we care to do so here?

Does it? If so, thats fine.

~Andrew

[PATCH 03/10] x86: don't build platform hypercall helpers multiple times

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 4398 bytes --]

... to eliminate the resulting duplicate symbols. This includes
dropping an odd per-CPU variable left from 32-bit days: Now that we
only care about 64-bit builds, converting the uint64_t needing
passing to a void pointer is no problem anymore.

Since the COMPAT handling section needs to be re-organized for this
anyway, also adjust a few other shortcomings (like declarations not
being visible at the point of the respective definition, risking both
to get out of sync).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -34,6 +34,20 @@
 #include "cpu/mtrr/mtrr.h"
 #include <xsm/xsm.h>
 
+/* Declarations for items shared with the compat mode handler. */
+extern spinlock_t xenpf_lock;
+
+#define RESOURCE_ACCESS_MAX_ENTRIES 3
+struct resource_access {
+    unsigned int nr_done;
+    unsigned int nr_entries;
+    xenpf_resource_entry_t *entries;
+};
+
+long cpu_frequency_change_helper(void *);
+void check_resource_access(struct resource_access *);
+void resource_access(void *);
+
 #ifndef COMPAT
 typedef long ret_t;
 DEFINE_SPINLOCK(xenpf_lock);
@@ -43,32 +57,12 @@ DEFINE_SPINLOCK(xenpf_lock);
 # define copy_to_compat copy_to_guest
 # undef guest_from_compat_handle
 # define guest_from_compat_handle(x,y) ((x)=(y))
-#else
-extern spinlock_t xenpf_lock;
-#endif
-
-static DEFINE_PER_CPU(uint64_t, freq);
 
-static long cpu_frequency_change_helper(void *data)
+long cpu_frequency_change_helper(void *data)
 {
-    return cpu_frequency_change(this_cpu(freq));
+    return cpu_frequency_change((uint64_t)data);
 }
 
-/* from sysctl.c */
-long cpu_up_helper(void *data);
-long cpu_down_helper(void *data);
-
-/* from core_parking.c */
-long core_parking_helper(void *data);
-uint32_t get_cur_idle_nums(void);
-
-#define RESOURCE_ACCESS_MAX_ENTRIES 3
-struct xen_resource_access {
-    unsigned int nr_done;
-    unsigned int nr_entries;
-    xenpf_resource_entry_t *entries;
-};
-
 static bool_t allow_access_msr(unsigned int msr)
 {
     switch ( msr )
@@ -83,7 +77,7 @@ static bool_t allow_access_msr(unsigned 
     return 0;
 }
 
-static void check_resource_access(struct xen_resource_access *ra)
+void check_resource_access(struct resource_access *ra)
 {
     unsigned int i;
 
@@ -122,9 +116,9 @@ static void check_resource_access(struct
     ra->nr_done = i;
 }
 
-static void resource_access(void *info)
+void resource_access(void *info)
 {
-    struct xen_resource_access *ra = info;
+    struct resource_access *ra = info;
     unsigned int i;
     u64 tsc = 0;
 
@@ -185,6 +179,7 @@ static void resource_access(void *info)
 
     ra->nr_done = i;
 }
+#endif
 
 ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
 {
@@ -457,10 +452,9 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
         ret = -EINVAL;
         if ( op->u.change_freq.flags || !cpu_online(op->u.change_freq.cpu) )
             break;
-        per_cpu(freq, op->u.change_freq.cpu) = op->u.change_freq.freq;
         ret = continue_hypercall_on_cpu(op->u.change_freq.cpu,
                                         cpu_frequency_change_helper,
-                                        NULL);
+                                        (void *)op->u.change_freq.freq);
         break;
 
     case XENPF_getidletime:
@@ -734,7 +728,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
 
     case XENPF_resource_op:
     {
-        struct xen_resource_access ra;
+        struct resource_access ra;
         unsigned int cpu;
         XEN_GUEST_HANDLE(xenpf_resource_entry_t) guest_entries;
 
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -24,6 +24,7 @@
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
 #include <asm/processor.h>
+#include <asm/smp.h>
 #include <asm/numa.h>
 #include <xen/nodemask.h>
 #include <xen/cpu.h>
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -58,6 +58,12 @@ int hard_smp_processor_id(void);
 
 void __stop_this_cpu(void);
 
+long cpu_up_helper(void *data);
+long cpu_down_helper(void *data);
+
+long core_parking_helper(void *data);
+uint32_t get_cur_idle_nums(void);
+
 /*
  * The value may be greater than the actual socket number in the system and
  * is required not to change from the initial startup.



[-- Attachment #2: x86-platform-helpers-once.patch --]
[-- Type: text/plain, Size: 4456 bytes --]

x86: don't build platform hypercall helpers multiple times

... to eliminate the resulting duplicate symbols. This includes
dropping an odd per-CPU variable left from 32-bit days: Now that we
only care about 64-bit builds, converting the uint64_t needing
passing to a void pointer is no problem anymore.

Since the COMPAT handling section needs to be re-organized for this
anyway, also adjust a few other shortcomings (like declarations not
being visible at the point of the respective definition, risking both
to get out of sync).

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -34,6 +34,20 @@
 #include "cpu/mtrr/mtrr.h"
 #include <xsm/xsm.h>
 
+/* Declarations for items shared with the compat mode handler. */
+extern spinlock_t xenpf_lock;
+
+#define RESOURCE_ACCESS_MAX_ENTRIES 3
+struct resource_access {
+    unsigned int nr_done;
+    unsigned int nr_entries;
+    xenpf_resource_entry_t *entries;
+};
+
+long cpu_frequency_change_helper(void *);
+void check_resource_access(struct resource_access *);
+void resource_access(void *);
+
 #ifndef COMPAT
 typedef long ret_t;
 DEFINE_SPINLOCK(xenpf_lock);
@@ -43,32 +57,12 @@ DEFINE_SPINLOCK(xenpf_lock);
 # define copy_to_compat copy_to_guest
 # undef guest_from_compat_handle
 # define guest_from_compat_handle(x,y) ((x)=(y))
-#else
-extern spinlock_t xenpf_lock;
-#endif
-
-static DEFINE_PER_CPU(uint64_t, freq);
 
-static long cpu_frequency_change_helper(void *data)
+long cpu_frequency_change_helper(void *data)
 {
-    return cpu_frequency_change(this_cpu(freq));
+    return cpu_frequency_change((uint64_t)data);
 }
 
-/* from sysctl.c */
-long cpu_up_helper(void *data);
-long cpu_down_helper(void *data);
-
-/* from core_parking.c */
-long core_parking_helper(void *data);
-uint32_t get_cur_idle_nums(void);
-
-#define RESOURCE_ACCESS_MAX_ENTRIES 3
-struct xen_resource_access {
-    unsigned int nr_done;
-    unsigned int nr_entries;
-    xenpf_resource_entry_t *entries;
-};
-
 static bool_t allow_access_msr(unsigned int msr)
 {
     switch ( msr )
@@ -83,7 +77,7 @@ static bool_t allow_access_msr(unsigned 
     return 0;
 }
 
-static void check_resource_access(struct xen_resource_access *ra)
+void check_resource_access(struct resource_access *ra)
 {
     unsigned int i;
 
@@ -122,9 +116,9 @@ static void check_resource_access(struct
     ra->nr_done = i;
 }
 
-static void resource_access(void *info)
+void resource_access(void *info)
 {
-    struct xen_resource_access *ra = info;
+    struct resource_access *ra = info;
     unsigned int i;
     u64 tsc = 0;
 
@@ -185,6 +179,7 @@ static void resource_access(void *info)
 
     ra->nr_done = i;
 }
+#endif
 
 ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
 {
@@ -457,10 +452,9 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
         ret = -EINVAL;
         if ( op->u.change_freq.flags || !cpu_online(op->u.change_freq.cpu) )
             break;
-        per_cpu(freq, op->u.change_freq.cpu) = op->u.change_freq.freq;
         ret = continue_hypercall_on_cpu(op->u.change_freq.cpu,
                                         cpu_frequency_change_helper,
-                                        NULL);
+                                        (void *)op->u.change_freq.freq);
         break;
 
     case XENPF_getidletime:
@@ -734,7 +728,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
 
     case XENPF_resource_op:
     {
-        struct xen_resource_access ra;
+        struct resource_access ra;
         unsigned int cpu;
         XEN_GUEST_HANDLE(xenpf_resource_entry_t) guest_entries;
 
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -24,6 +24,7 @@
 #include <asm/hvm/hvm.h>
 #include <asm/hvm/support.h>
 #include <asm/processor.h>
+#include <asm/smp.h>
 #include <asm/numa.h>
 #include <xen/nodemask.h>
 #include <xen/cpu.h>
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -58,6 +58,12 @@ int hard_smp_processor_id(void);
 
 void __stop_this_cpu(void);
 
+long cpu_up_helper(void *data);
+long cpu_down_helper(void *data);
+
+long core_parking_helper(void *data);
+uint32_t get_cur_idle_nums(void);
+
 /*
  * The value may be greater than the actual socket number in the system and
  * is required not to change from the initial startup.

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 03/10] x86: don't build platform hypercall helpers multiple times

[Andrew Cooper]

On 20/10/15 11:40, Jan Beulich wrote:
> ... to eliminate the resulting duplicate symbols. This includes
> dropping an odd per-CPU variable left from 32-bit days: Now that we
> only care about 64-bit builds, converting the uint64_t needing
> passing to a void pointer is no problem anymore.
>
> Since the COMPAT handling section needs to be re-organized for this
> anyway, also adjust a few other shortcomings (like declarations not
> being visible at the point of the respective definition, risking both
> to get out of sync).
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

[PATCH 04/10] x86/HVM: prefix both instances of enable_intr_window()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3831 bytes --]

... to tell them apart by their names even without further context.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -75,7 +75,7 @@ static void svm_inject_extint(struct vcp
     vmcb->eventinj = event;
 }
 
-static void enable_intr_window(struct vcpu *v, struct hvm_intack intack)
+static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     uint32_t general1_intercepts = vmcb_get_general1_intercepts(vmcb);
@@ -195,7 +195,7 @@ void svm_intr_assist(void) 
          */
         if ( unlikely(vmcb->eventinj.fields.v) || intblk )
         {
-            enable_intr_window(v, intack);
+            svm_enable_intr_window(v, intack);
             return;
         }
 
@@ -216,7 +216,7 @@ void svm_intr_assist(void) 
     /* Is there another IRQ to queue up behind this one? */
     intack = hvm_vcpu_has_pending_irq(v);
     if ( unlikely(intack.source != hvm_intsrc_none) )
-        enable_intr_window(v, intack);
+        svm_enable_intr_window(v, intack);
 }
 
 /*
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -67,7 +67,7 @@
  * the STI- and MOV-SS-blocking interruptibility-state flags.
  */
 
-static void enable_intr_window(struct vcpu *v, struct hvm_intack intack)
+static void vmx_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
 {
     u32 ctl = CPU_BASED_VIRTUAL_INTR_PENDING;
 
@@ -182,7 +182,7 @@ static int nvmx_intr_intercept(struct vc
 
     if ( nvmx_intr_blocked(v) != hvm_intblk_none )
     {
-        enable_intr_window(v, intack);
+        vmx_enable_intr_window(v, intack);
         return 1;
     }
 
@@ -206,10 +206,10 @@ static int nvmx_intr_intercept(struct vc
 
                 intack = hvm_vcpu_has_pending_irq(v);
                 if ( unlikely(intack.source != hvm_intsrc_none) )
-                    enable_intr_window(v, intack);
+                    vmx_enable_intr_window(v, intack);
             }
             else
-                enable_intr_window(v, intack);
+                vmx_enable_intr_window(v, intack);
 
             return 1;
         }
@@ -257,7 +257,7 @@ void vmx_intr_assist(void)
                   intack.source == hvm_intsrc_vector ||
                   intack.source == hvm_intsrc_nmi) )
             {
-                enable_intr_window(v, intack);
+                vmx_enable_intr_window(v, intack);
                 goto out;
             }
 
@@ -267,7 +267,7 @@ void vmx_intr_assist(void)
                 if ( (intack.source == hvm_intsrc_pic) ||
                      (intack.source == hvm_intsrc_nmi) ||
                      (intack.source == hvm_intsrc_mce) )
-                    enable_intr_window(v, intack);
+                    vmx_enable_intr_window(v, intack);
 
                 goto out;
             }
@@ -280,7 +280,7 @@ void vmx_intr_assist(void)
         }
         else if ( intblk != hvm_intblk_none )
         {
-            enable_intr_window(v, intack);
+            vmx_enable_intr_window(v, intack);
             goto out;
         }
         else
@@ -288,7 +288,7 @@ void vmx_intr_assist(void)
             __vmread(VM_ENTRY_INTR_INFO, &intr_info);
             if ( intr_info & INTR_INFO_VALID_MASK )
             {
-                enable_intr_window(v, intack);
+                vmx_enable_intr_window(v, intack);
                 goto out;
             }
         }
@@ -350,7 +350,7 @@ void vmx_intr_assist(void)
          intack.source == hvm_intsrc_vector )
     {
         if ( unlikely(intack.source != hvm_intsrc_none) )
-            enable_intr_window(v, intack);
+            vmx_enable_intr_window(v, intack);
     }
 
  out:




[-- Attachment #2: x86-HVM-enable_intr_window-unique.patch --]
[-- Type: text/plain, Size: 3883 bytes --]

x86/HVM: prefix both instances of enable_intr_window()

... to tell them apart by their names even without further context.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/svm/intr.c
+++ b/xen/arch/x86/hvm/svm/intr.c
@@ -75,7 +75,7 @@ static void svm_inject_extint(struct vcp
     vmcb->eventinj = event;
 }
 
-static void enable_intr_window(struct vcpu *v, struct hvm_intack intack)
+static void svm_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
 {
     struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
     uint32_t general1_intercepts = vmcb_get_general1_intercepts(vmcb);
@@ -195,7 +195,7 @@ void svm_intr_assist(void) 
          */
         if ( unlikely(vmcb->eventinj.fields.v) || intblk )
         {
-            enable_intr_window(v, intack);
+            svm_enable_intr_window(v, intack);
             return;
         }
 
@@ -216,7 +216,7 @@ void svm_intr_assist(void) 
     /* Is there another IRQ to queue up behind this one? */
     intack = hvm_vcpu_has_pending_irq(v);
     if ( unlikely(intack.source != hvm_intsrc_none) )
-        enable_intr_window(v, intack);
+        svm_enable_intr_window(v, intack);
 }
 
 /*
--- a/xen/arch/x86/hvm/vmx/intr.c
+++ b/xen/arch/x86/hvm/vmx/intr.c
@@ -67,7 +67,7 @@
  * the STI- and MOV-SS-blocking interruptibility-state flags.
  */
 
-static void enable_intr_window(struct vcpu *v, struct hvm_intack intack)
+static void vmx_enable_intr_window(struct vcpu *v, struct hvm_intack intack)
 {
     u32 ctl = CPU_BASED_VIRTUAL_INTR_PENDING;
 
@@ -182,7 +182,7 @@ static int nvmx_intr_intercept(struct vc
 
     if ( nvmx_intr_blocked(v) != hvm_intblk_none )
     {
-        enable_intr_window(v, intack);
+        vmx_enable_intr_window(v, intack);
         return 1;
     }
 
@@ -206,10 +206,10 @@ static int nvmx_intr_intercept(struct vc
 
                 intack = hvm_vcpu_has_pending_irq(v);
                 if ( unlikely(intack.source != hvm_intsrc_none) )
-                    enable_intr_window(v, intack);
+                    vmx_enable_intr_window(v, intack);
             }
             else
-                enable_intr_window(v, intack);
+                vmx_enable_intr_window(v, intack);
 
             return 1;
         }
@@ -257,7 +257,7 @@ void vmx_intr_assist(void)
                   intack.source == hvm_intsrc_vector ||
                   intack.source == hvm_intsrc_nmi) )
             {
-                enable_intr_window(v, intack);
+                vmx_enable_intr_window(v, intack);
                 goto out;
             }
 
@@ -267,7 +267,7 @@ void vmx_intr_assist(void)
                 if ( (intack.source == hvm_intsrc_pic) ||
                      (intack.source == hvm_intsrc_nmi) ||
                      (intack.source == hvm_intsrc_mce) )
-                    enable_intr_window(v, intack);
+                    vmx_enable_intr_window(v, intack);
 
                 goto out;
             }
@@ -280,7 +280,7 @@ void vmx_intr_assist(void)
         }
         else if ( intblk != hvm_intblk_none )
         {
-            enable_intr_window(v, intack);
+            vmx_enable_intr_window(v, intack);
             goto out;
         }
         else
@@ -288,7 +288,7 @@ void vmx_intr_assist(void)
             __vmread(VM_ENTRY_INTR_INFO, &intr_info);
             if ( intr_info & INTR_INFO_VALID_MASK )
             {
-                enable_intr_window(v, intack);
+                vmx_enable_intr_window(v, intack);
                 goto out;
             }
         }
@@ -350,7 +350,7 @@ void vmx_intr_assist(void)
          intack.source == hvm_intsrc_vector )
     {
         if ( unlikely(intack.source != hvm_intsrc_none) )
-            enable_intr_window(v, intack);
+            vmx_enable_intr_window(v, intack);
     }
 
  out:

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 04/10] x86/HVM: prefix both instances of enable_intr_window()

[Andrew Cooper]

On 20/10/15 11:41, Jan Beulich wrote:
> ... to tell them apart by their names even without further context.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH 04/10] x86/HVM: prefix both instances of enable_intr_window()

[Aravind Gopalakrishnan]

On 10/20/2015 5:41 AM, Jan Beulich wrote:
> ... to tell them apart by their names even without further context.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>

Reviewed-by: Aravind Gopalakrishnan<Aravind.Gopalakrishnan@amd.com>

Thanks,
-Aravind.

Re: [PATCH 04/10] x86/HVM: prefix both instances of enable_intr_window()

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Tuesday, October 20, 2015 6:41 PM
> 
> ... to tell them apart by their names even without further context.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 

Acked-by: Kevin Tian <kevin.tian@intel.com>

[PATCH 05/10] x86/mm: build map_domain_gfn() just once

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1636 bytes --]

It doesn't depend on GUEST_PAGING_LEVELS.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: To ensure no dependency on GUEST_PAGING_LEVELS, would it perhaps
     be better to move the function to a file compiled just once?

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -87,8 +87,11 @@ static uint32_t set_ad_bits(void *guest_
     return 0;
 }
 
-/* If the map is non-NULL, we leave this function having 
- * acquired an extra ref on mfn_to_page(*mfn) */
+#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
+/*
+ * If the map is non-NULL, we leave this function having
+ * acquired an extra ref on mfn_to_page(*mfn).
+ */
 void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
                      p2m_type_t *p2mt, p2m_query_t q, uint32_t *rc)
 {
@@ -125,7 +128,7 @@ void *map_domain_gfn(struct p2m_domain *
     map = map_domain_page(*mfn);
     return map;
 }
-
+#endif
 
 /* Walk the guest pagetables, after the manner of a hardware walker. */
 /* Because the walk is essentially random, it can cause a deadlock 
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -305,7 +305,6 @@ guest_walk_to_page_order(walk_t *gw)
 #define GPT_RENAME2(_n, _l) _n ## _ ## _l ## _levels
 #define GPT_RENAME(_n, _l) GPT_RENAME2(_n, _l)
 #define guest_walk_tables GPT_RENAME(guest_walk_tables, GUEST_PAGING_LEVELS)
-#define map_domain_gfn GPT_RENAME(map_domain_gfn, GUEST_PAGING_LEVELS)
 
 void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
                      p2m_type_t *p2mt, p2m_query_t q, uint32_t *rc);




[-- Attachment #2: x86-map_domain_gfn-once.patch --]
[-- Type: text/plain, Size: 1674 bytes --]

x86/mm: build map_domain_gfn() just once

It doesn't depend on GUEST_PAGING_LEVELS.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: To ensure no dependency on GUEST_PAGING_LEVELS, would it perhaps
     be better to move the function to a file compiled just once?

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -87,8 +87,11 @@ static uint32_t set_ad_bits(void *guest_
     return 0;
 }
 
-/* If the map is non-NULL, we leave this function having 
- * acquired an extra ref on mfn_to_page(*mfn) */
+#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
+/*
+ * If the map is non-NULL, we leave this function having
+ * acquired an extra ref on mfn_to_page(*mfn).
+ */
 void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
                      p2m_type_t *p2mt, p2m_query_t q, uint32_t *rc)
 {
@@ -125,7 +128,7 @@ void *map_domain_gfn(struct p2m_domain *
     map = map_domain_page(*mfn);
     return map;
 }
-
+#endif
 
 /* Walk the guest pagetables, after the manner of a hardware walker. */
 /* Because the walk is essentially random, it can cause a deadlock 
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -305,7 +305,6 @@ guest_walk_to_page_order(walk_t *gw)
 #define GPT_RENAME2(_n, _l) _n ## _ ## _l ## _levels
 #define GPT_RENAME(_n, _l) GPT_RENAME2(_n, _l)
 #define guest_walk_tables GPT_RENAME(guest_walk_tables, GUEST_PAGING_LEVELS)
-#define map_domain_gfn GPT_RENAME(map_domain_gfn, GUEST_PAGING_LEVELS)
 
 void *map_domain_gfn(struct p2m_domain *p2m, gfn_t gfn, mfn_t *mfn,
                      p2m_type_t *p2mt, p2m_query_t q, uint32_t *rc);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 05/10] x86/mm: build map_domain_gfn() just once

[Andrew Cooper]

On 20/10/15 11:41, Jan Beulich wrote:
> It doesn't depend on GUEST_PAGING_LEVELS.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: To ensure no dependency on GUEST_PAGING_LEVELS, would it perhaps
>      be better to move the function to a file compiled just once?

+1  it probably fits best in p2m.c, although none of the surrounding
files is a particularly good fit.

If there are other generic helpers, it might be worth making a common.c

~Andrew

Re: [PATCH 05/10] x86/mm: build map_domain_gfn() just once

[Jan Beulich]

>>> On 20.10.15 at 14:54, <andrew.cooper3@citrix.com> wrote:
> On 20/10/15 11:41, Jan Beulich wrote:
>> It doesn't depend on GUEST_PAGING_LEVELS.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: To ensure no dependency on GUEST_PAGING_LEVELS, would it perhaps
>>      be better to move the function to a file compiled just once?
> 
> +1  it probably fits best in p2m.c, although none of the surrounding
> files is a particularly good fit.
> 
> If there are other generic helpers, it might be worth making a common.c

That's the thing - I didn't spot any, and for just this function I didn't
want to introduce a new one.

Jan

[PATCH 06/10] x86/mm: only a single instance of gw_page_flags[] is needed

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 3155 bytes --]

None of its elements depends on GUEST_PAGING_LEVELS.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -32,29 +32,32 @@
 /* Allow uniquely identifying static symbols in the 3 generated objects. */
 asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
 
+extern const uint32_t gw_page_flags[];
+#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
+const uint32_t gw_page_flags[] = {
+    /* I/F -  Usr Wr */
+    /* 0   0   0   0 */ _PAGE_PRESENT,
+    /* 0   0   0   1 */ _PAGE_PRESENT|_PAGE_RW,
+    /* 0   0   1   0 */ _PAGE_PRESENT|_PAGE_USER,
+    /* 0   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
+    /* 0   1   0   0 */ _PAGE_PRESENT,
+    /* 0   1   0   1 */ _PAGE_PRESENT|_PAGE_RW,
+    /* 0   1   1   0 */ _PAGE_PRESENT|_PAGE_USER,
+    /* 0   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
+    /* 1   0   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT,
+    /* 1   0   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
+    /* 1   0   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
+    /* 1   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
+    /* 1   1   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT,
+    /* 1   1   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
+    /* 1   1   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
+    /* 1   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
+};
+#endif
+
 /* Flags that are needed in a pagetable entry, with the sense of NX inverted */
 static uint32_t mandatory_flags(struct vcpu *v, uint32_t pfec) 
 {
-    static const uint32_t flags[] = {
-        /* I/F -  Usr Wr */
-        /* 0   0   0   0 */ _PAGE_PRESENT, 
-        /* 0   0   0   1 */ _PAGE_PRESENT|_PAGE_RW,
-        /* 0   0   1   0 */ _PAGE_PRESENT|_PAGE_USER,
-        /* 0   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
-        /* 0   1   0   0 */ _PAGE_PRESENT, 
-        /* 0   1   0   1 */ _PAGE_PRESENT|_PAGE_RW,
-        /* 0   1   1   0 */ _PAGE_PRESENT|_PAGE_USER,
-        /* 0   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
-        /* 1   0   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT, 
-        /* 1   0   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
-        /* 1   0   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
-        /* 1   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
-        /* 1   1   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT, 
-        /* 1   1   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
-        /* 1   1   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
-        /* 1   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
-    };
-
     /* Don't demand not-NX if the CPU wouldn't enforce it. */
     if ( !guest_supports_nx(v) )
         pfec &= ~PFEC_insn_fetch;
@@ -64,7 +67,7 @@ static uint32_t mandatory_flags(struct v
          && !(pfec & PFEC_user_mode) )
         pfec &= ~PFEC_write_access;
 
-    return flags[(pfec & 0x1f) >> 1] | _PAGE_INVALID_BITS;
+    return gw_page_flags[(pfec & 0x1f) >> 1] | _PAGE_INVALID_BITS;
 }
 
 /* Modify a guest pagetable entry to set the Accessed and Dirty bits.




[-- Attachment #2: x86-gw-page-flags.patch --]
[-- Type: text/plain, Size: 3212 bytes --]

x86/mm: only a single instance of gw_page_flags[] is needed

None of its elements depends on GUEST_PAGING_LEVELS.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -32,29 +32,32 @@
 /* Allow uniquely identifying static symbols in the 3 generated objects. */
 asm(".file \"guest_walk_" __stringify(GUEST_PAGING_LEVELS) ".o\"");
 
+extern const uint32_t gw_page_flags[];
+#if GUEST_PAGING_LEVELS == CONFIG_PAGING_LEVELS
+const uint32_t gw_page_flags[] = {
+    /* I/F -  Usr Wr */
+    /* 0   0   0   0 */ _PAGE_PRESENT,
+    /* 0   0   0   1 */ _PAGE_PRESENT|_PAGE_RW,
+    /* 0   0   1   0 */ _PAGE_PRESENT|_PAGE_USER,
+    /* 0   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
+    /* 0   1   0   0 */ _PAGE_PRESENT,
+    /* 0   1   0   1 */ _PAGE_PRESENT|_PAGE_RW,
+    /* 0   1   1   0 */ _PAGE_PRESENT|_PAGE_USER,
+    /* 0   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
+    /* 1   0   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT,
+    /* 1   0   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
+    /* 1   0   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
+    /* 1   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
+    /* 1   1   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT,
+    /* 1   1   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
+    /* 1   1   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
+    /* 1   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
+};
+#endif
+
 /* Flags that are needed in a pagetable entry, with the sense of NX inverted */
 static uint32_t mandatory_flags(struct vcpu *v, uint32_t pfec) 
 {
-    static const uint32_t flags[] = {
-        /* I/F -  Usr Wr */
-        /* 0   0   0   0 */ _PAGE_PRESENT, 
-        /* 0   0   0   1 */ _PAGE_PRESENT|_PAGE_RW,
-        /* 0   0   1   0 */ _PAGE_PRESENT|_PAGE_USER,
-        /* 0   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
-        /* 0   1   0   0 */ _PAGE_PRESENT, 
-        /* 0   1   0   1 */ _PAGE_PRESENT|_PAGE_RW,
-        /* 0   1   1   0 */ _PAGE_PRESENT|_PAGE_USER,
-        /* 0   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER,
-        /* 1   0   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT, 
-        /* 1   0   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
-        /* 1   0   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
-        /* 1   0   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
-        /* 1   1   0   0 */ _PAGE_PRESENT|_PAGE_NX_BIT, 
-        /* 1   1   0   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_NX_BIT,
-        /* 1   1   1   0 */ _PAGE_PRESENT|_PAGE_USER|_PAGE_NX_BIT,
-        /* 1   1   1   1 */ _PAGE_PRESENT|_PAGE_RW|_PAGE_USER|_PAGE_NX_BIT,
-    };
-
     /* Don't demand not-NX if the CPU wouldn't enforce it. */
     if ( !guest_supports_nx(v) )
         pfec &= ~PFEC_insn_fetch;
@@ -64,7 +67,7 @@ static uint32_t mandatory_flags(struct v
          && !(pfec & PFEC_user_mode) )
         pfec &= ~PFEC_write_access;
 
-    return flags[(pfec & 0x1f) >> 1] | _PAGE_INVALID_BITS;
+    return gw_page_flags[(pfec & 0x1f) >> 1] | _PAGE_INVALID_BITS;
 }
 
 /* Modify a guest pagetable entry to set the Accessed and Dirty bits.

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 06/10] x86/mm: only a single instance of gw_page_flags[] is needed

[Andrew Cooper]

On 20/10/15 11:42, Jan Beulich wrote:
> None of its elements depends on GUEST_PAGING_LEVELS.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

If you do end up making a common.c (as suggested in patch 5), then this
is also a candidate to move, to avoid further issues with multiple
compilations.

[PATCH 07/10] x86/shadow: only a single instance of fetch_type_names[] is needed

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -76,8 +76,10 @@ typedef enum {
     ft_demand_write = FETCH_TYPE_DEMAND | FETCH_TYPE_WRITE,
 } fetch_type_t;
 
-#ifdef DEBUG_TRACE_DUMP
-static char *fetch_type_names[] = {
+extern const char *const fetch_type_names[];
+
+#if defined(DEBUG_TRACE_DUMP) && CONFIG_PAGING_LEVELS == GUEST_PAGING_LEVELS
+const char *const fetch_type_names[] = {
     [ft_prefetch]     "prefetch",
     [ft_demand_read]  "demand read",
     [ft_demand_write] "demand write",




[-- Attachment #2: x86-sh-fetch_type_names-once.patch --]
[-- Type: text/plain, Size: 665 bytes --]

x86/shadow: only a single instance of fetch_type_names[] is needed

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -76,8 +76,10 @@ typedef enum {
     ft_demand_write = FETCH_TYPE_DEMAND | FETCH_TYPE_WRITE,
 } fetch_type_t;
 
-#ifdef DEBUG_TRACE_DUMP
-static char *fetch_type_names[] = {
+extern const char *const fetch_type_names[];
+
+#if defined(DEBUG_TRACE_DUMP) && CONFIG_PAGING_LEVELS == GUEST_PAGING_LEVELS
+const char *const fetch_type_names[] = {
     [ft_prefetch]     "prefetch",
     [ft_demand_read]  "demand read",
     [ft_demand_write] "demand write",

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 07/10] x86/shadow: only a single instance of fetch_type_names[] is needed

[Tim Deegan]

At 04:43 -0600 on 20 Oct (1445316214), Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

[PATCH 08/10] x86/shadow: adjust sh_{make, destroy}_monitor_table() name tags

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 2301 bytes --]

Instead of the misleading _guest_<level> ones use _sh_<level>,
expressing their sole dependency on the number of shadow levels.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.h
+++ b/xen/arch/x86/mm/shadow/multi.h
@@ -105,13 +105,6 @@ extern void
 SHADOW_INTERNAL_NAME(sh_guest_get_eff_l1e, GUEST_LEVELS)
     (struct vcpu *v, unsigned long va, void *eff_l1e);
 
-extern mfn_t
-SHADOW_INTERNAL_NAME(sh_make_monitor_table, GUEST_LEVELS)
-    (struct vcpu *v);
-extern void
-SHADOW_INTERNAL_NAME(sh_destroy_monitor_table, GUEST_LEVELS)
-    (struct vcpu *v, mfn_t mmfn);
-
 extern const struct paging_mode
 SHADOW_INTERNAL_NAME(sh_paging_mode, GUEST_LEVELS);
 
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -151,10 +151,12 @@ extern void shadow_audit_tables(struct v
  * Macro for dealing with the naming of the internal names of the
  * shadow code's external entry points.
  */
-#define SHADOW_INTERNAL_NAME_HIDDEN(name, guest_levels) \
-    name ## __guest_ ## guest_levels
+#define SHADOW_INTERNAL_NAME_(name, kind, value)        \
+    name ## __ ## kind ## _ ## value
 #define SHADOW_INTERNAL_NAME(name, guest_levels)        \
-    SHADOW_INTERNAL_NAME_HIDDEN(name, guest_levels)
+    SHADOW_INTERNAL_NAME_(name, guest, guest_levels)
+#define SHADOW_SH_NAME(name, shadow_levels)             \
+    SHADOW_INTERNAL_NAME_(name, sh, shadow_levels)
 
 #define GUEST_LEVELS  2
 #include "multi.h"
--- a/xen/arch/x86/mm/shadow/types.h
+++ b/xen/arch/x86/mm/shadow/types.h
@@ -270,9 +270,12 @@ static inline shadow_l4e_t shadow_l4e_fr
 
 /* sh_make_monitor_table depends only on the number of shadow levels */
 #define sh_make_monitor_table \
-        SHADOW_INTERNAL_NAME(sh_make_monitor_table, SHADOW_PAGING_LEVELS)
+        SHADOW_SH_NAME(sh_make_monitor_table, SHADOW_PAGING_LEVELS)
 #define sh_destroy_monitor_table \
-        SHADOW_INTERNAL_NAME(sh_destroy_monitor_table, SHADOW_PAGING_LEVELS)
+        SHADOW_SH_NAME(sh_destroy_monitor_table, SHADOW_PAGING_LEVELS)
+
+mfn_t sh_make_monitor_table(struct vcpu *v);
+void sh_destroy_monitor_table(struct vcpu *v, mfn_t mmfn);
 
 #if SHADOW_PAGING_LEVELS == 3
 #define MFN_FITS_IN_HVM_CR3(_MFN) !(mfn_x(_MFN) >> 20)




[-- Attachment #2: x86-sh-monitor-table-width.patch --]
[-- Type: text/plain, Size: 2361 bytes --]

x86/shadow: adjust sh_{make,destroy}_monitor_table() name tags

Instead of the misleading _guest_<level> ones use _sh_<level>,
expressing their sole dependency on the number of shadow levels.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/multi.h
+++ b/xen/arch/x86/mm/shadow/multi.h
@@ -105,13 +105,6 @@ extern void
 SHADOW_INTERNAL_NAME(sh_guest_get_eff_l1e, GUEST_LEVELS)
     (struct vcpu *v, unsigned long va, void *eff_l1e);
 
-extern mfn_t
-SHADOW_INTERNAL_NAME(sh_make_monitor_table, GUEST_LEVELS)
-    (struct vcpu *v);
-extern void
-SHADOW_INTERNAL_NAME(sh_destroy_monitor_table, GUEST_LEVELS)
-    (struct vcpu *v, mfn_t mmfn);
-
 extern const struct paging_mode
 SHADOW_INTERNAL_NAME(sh_paging_mode, GUEST_LEVELS);
 
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -151,10 +151,12 @@ extern void shadow_audit_tables(struct v
  * Macro for dealing with the naming of the internal names of the
  * shadow code's external entry points.
  */
-#define SHADOW_INTERNAL_NAME_HIDDEN(name, guest_levels) \
-    name ## __guest_ ## guest_levels
+#define SHADOW_INTERNAL_NAME_(name, kind, value)        \
+    name ## __ ## kind ## _ ## value
 #define SHADOW_INTERNAL_NAME(name, guest_levels)        \
-    SHADOW_INTERNAL_NAME_HIDDEN(name, guest_levels)
+    SHADOW_INTERNAL_NAME_(name, guest, guest_levels)
+#define SHADOW_SH_NAME(name, shadow_levels)             \
+    SHADOW_INTERNAL_NAME_(name, sh, shadow_levels)
 
 #define GUEST_LEVELS  2
 #include "multi.h"
--- a/xen/arch/x86/mm/shadow/types.h
+++ b/xen/arch/x86/mm/shadow/types.h
@@ -270,9 +270,12 @@ static inline shadow_l4e_t shadow_l4e_fr
 
 /* sh_make_monitor_table depends only on the number of shadow levels */
 #define sh_make_monitor_table \
-        SHADOW_INTERNAL_NAME(sh_make_monitor_table, SHADOW_PAGING_LEVELS)
+        SHADOW_SH_NAME(sh_make_monitor_table, SHADOW_PAGING_LEVELS)
 #define sh_destroy_monitor_table \
-        SHADOW_INTERNAL_NAME(sh_destroy_monitor_table, SHADOW_PAGING_LEVELS)
+        SHADOW_SH_NAME(sh_destroy_monitor_table, SHADOW_PAGING_LEVELS)
+
+mfn_t sh_make_monitor_table(struct vcpu *v);
+void sh_destroy_monitor_table(struct vcpu *v, mfn_t mmfn);
 
 #if SHADOW_PAGING_LEVELS == 3
 #define MFN_FITS_IN_HVM_CR3(_MFN) !(mfn_x(_MFN) >> 20)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 08/10] x86/shadow: adjust sh_{make, destroy}_monitor_table() name tags

[Tim Deegan]

At 04:44 -0600 on 20 Oct (1445316259), Jan Beulich wrote:
> Instead of the misleading _guest_<level> ones use _sh_<level>,
> expressing their sole dependency on the number of shadow levels.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

[PATCH 09/10] x86/shadow: drop stray name tags from sh_{guest_get, map}_eff_l1e()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 7778 bytes --]

They (as a now being removed comment validly says) depend only on Xen's
number of page table levels, and hence their tags didn't serve any
useful purpose (there could only ever be one instance in a single
binary, even back in the x86-32 days).

Further conditionalize the inclusion of PV-specific hook pointers, at
once making sure that PV guests can't ever get other than 4-level mode
enabled for them.

For consistency reasons shadow_{write,cmpxchg}_guest_entry() also get
moved next to the other PV-only actors, allowing them to become static
just like the $subject ones do.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -84,7 +84,9 @@ void shadow_vcpu_init(struct vcpu *v)
     }
 #endif
 
-    v->arch.paging.mode = &SHADOW_INTERNAL_NAME(sh_paging_mode, 3);
+    v->arch.paging.mode = is_pv_vcpu(v) ?
+                          &SHADOW_INTERNAL_NAME(sh_paging_mode, 4) :
+                          &SHADOW_INTERNAL_NAME(sh_paging_mode, 3);
 }
 
 #if SHADOW_AUDIT
@@ -1128,38 +1130,6 @@ sh_validate_guest_pt_write(struct vcpu *
     }
 }
 
-int shadow_write_guest_entry(struct vcpu *v, intpte_t *p,
-                             intpte_t new, mfn_t gmfn)
-/* Write a new value into the guest pagetable, and update the shadows
- * appropriately.  Returns 0 if we page-faulted, 1 for success. */
-{
-    int failed;
-    paging_lock(v->domain);
-    failed = __copy_to_user(p, &new, sizeof(new));
-    if ( failed != sizeof(new) )
-        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
-    paging_unlock(v->domain);
-    return (failed == 0);
-}
-
-int shadow_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p,
-                               intpte_t *old, intpte_t new, mfn_t gmfn)
-/* Cmpxchg a new value into the guest pagetable, and update the shadows
- * appropriately. Returns 0 if we page-faulted, 1 if not.
- * N.B. caller should check the value of "old" to see if the
- * cmpxchg itself was successful. */
-{
-    int failed;
-    intpte_t t = *old;
-    paging_lock(v->domain);
-    failed = cmpxchg_user(p, t, new);
-    if ( t == *old )
-        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
-    *old = t;
-    paging_unlock(v->domain);
-    return (failed == 0);
-}
-
 
 /**************************************************************************/
 /* Memory management for shadow pages. */
@@ -2782,14 +2752,7 @@ static void sh_update_paging_modes(struc
     if ( v->arch.paging.mode )
         v->arch.paging.mode->shadow.detach_old_tables(v);
 
-    if ( is_pv_domain(d) )
-    {
-        ///
-        /// PV guest
-        ///
-        v->arch.paging.mode = &SHADOW_INTERNAL_NAME(sh_paging_mode, 4);
-    }
-    else
+    if ( !is_pv_domain(d) )
     {
         ///
         /// HVM guest
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -371,7 +371,7 @@ static void sh_audit_gw(struct vcpu *v, 
 
 
 #if (CONFIG_PAGING_LEVELS == GUEST_PAGING_LEVELS)
-void *
+static void *
 sh_guest_map_l1e(struct vcpu *v, unsigned long addr,
                   unsigned long *gl1mfn)
 {
@@ -395,7 +395,7 @@ sh_guest_map_l1e(struct vcpu *v, unsigne
     return pl1e;
 }
 
-void
+static void
 sh_guest_get_eff_l1e(struct vcpu *v, unsigned long addr, void *eff_l1e)
 {
     walk_t gw;
@@ -408,6 +408,48 @@ sh_guest_get_eff_l1e(struct vcpu *v, uns
     (void) sh_walk_guest_tables(v, addr, &gw, PFEC_page_present);
     *(guest_l1e_t *)eff_l1e = gw.l1e;
 }
+
+/*
+ * Write a new value into the guest pagetable, and update the shadows
+ * appropriately.  Returns 0 if we page-faulted, 1 for success.
+ */
+static int
+sh_write_guest_entry(struct vcpu *v, guest_intpte_t *p,
+                     guest_intpte_t new, mfn_t gmfn)
+{
+    int failed;
+
+    paging_lock(v->domain);
+    failed = __copy_to_user(p, &new, sizeof(new));
+    if ( failed != sizeof(new) )
+        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
+    paging_unlock(v->domain);
+
+    return !failed;
+}
+
+/*
+ * Cmpxchg a new value into the guest pagetable, and update the shadows
+ * appropriately. Returns 0 if we page-faulted, 1 if not.
+ * N.B. caller should check the value of "old" to see if the cmpxchg itself
+ * was successful.
+ */
+static int
+sh_cmpxchg_guest_entry(struct vcpu *v, guest_intpte_t *p,
+                       guest_intpte_t *old, guest_intpte_t new, mfn_t gmfn)
+{
+    int failed;
+    guest_intpte_t t = *old;
+
+    paging_lock(v->domain);
+    failed = cmpxchg_user(p, t, new);
+    if ( t == *old )
+        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
+    *old = t;
+    paging_unlock(v->domain);
+
+    return !failed;
+}
 #endif /* CONFIG == GUEST (== SHADOW) */
 
 /**************************************************************************/
@@ -5184,10 +5226,12 @@ const struct paging_mode sh_paging_mode 
     .update_cr3                    = sh_update_cr3,
     .update_paging_modes           = shadow_update_paging_modes,
     .write_p2m_entry               = shadow_write_p2m_entry,
-    .write_guest_entry             = shadow_write_guest_entry,
-    .cmpxchg_guest_entry           = shadow_cmpxchg_guest_entry,
+#if CONFIG_PAGING_LEVELS == GUEST_PAGING_LEVELS
+    .write_guest_entry             = sh_write_guest_entry,
+    .cmpxchg_guest_entry           = sh_cmpxchg_guest_entry,
     .guest_map_l1e                 = sh_guest_map_l1e,
     .guest_get_eff_l1e             = sh_guest_get_eff_l1e,
+#endif
     .guest_levels                  = GUEST_PAGING_LEVELS,
     .shadow.detach_old_tables      = sh_detach_old_tables,
     .shadow.x86_emulate_write      = sh_x86_emulate_write,
--- a/xen/arch/x86/mm/shadow/multi.h
+++ b/xen/arch/x86/mm/shadow/multi.h
@@ -98,13 +98,6 @@ SHADOW_INTERNAL_NAME(sh_audit_l4_table, 
     (struct vcpu *v, mfn_t sl4mfn, mfn_t x);
 #endif
 
-extern void *
-SHADOW_INTERNAL_NAME(sh_guest_map_l1e, GUEST_LEVELS)
-    (struct vcpu *v, unsigned long va, unsigned long *gl1mfn);
-extern void
-SHADOW_INTERNAL_NAME(sh_guest_get_eff_l1e, GUEST_LEVELS)
-    (struct vcpu *v, unsigned long va, void *eff_l1e);
-
 extern const struct paging_mode
 SHADOW_INTERNAL_NAME(sh_paging_mode, GUEST_LEVELS);
 
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -383,10 +383,6 @@ extern int sh_remove_write_access(struct
 void shadow_write_p2m_entry(struct domain *d, unsigned long gfn,
                             l1_pgentry_t *p, l1_pgentry_t new,
                             unsigned int level);
-int shadow_write_guest_entry(struct vcpu *v, intpte_t *p,
-                             intpte_t new, mfn_t gmfn);
-int shadow_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p,
-                               intpte_t *old, intpte_t new, mfn_t gmfn);
 
 /* Update all the things that are derived from the guest's CR0/CR3/CR4.
  * Called to initialize paging structures if the paging mode
--- a/xen/arch/x86/mm/shadow/types.h
+++ b/xen/arch/x86/mm/shadow/types.h
@@ -262,12 +262,6 @@ static inline shadow_l4e_t shadow_l4e_fr
 #define sh_rm_write_access_from_sl1p INTERNAL_NAME(sh_rm_write_access_from_sl1p)
 #endif
 
-/* The sh_guest_(map|get)_* functions depends on Xen's paging levels */
-#define sh_guest_map_l1e \
-        SHADOW_INTERNAL_NAME(sh_guest_map_l1e, CONFIG_PAGING_LEVELS)
-#define sh_guest_get_eff_l1e \
-        SHADOW_INTERNAL_NAME(sh_guest_get_eff_l1e, CONFIG_PAGING_LEVELS)
-
 /* sh_make_monitor_table depends only on the number of shadow levels */
 #define sh_make_monitor_table \
         SHADOW_SH_NAME(sh_make_monitor_table, SHADOW_PAGING_LEVELS)



[-- Attachment #2: x86-sh-pv-guest-l1e.patch --]
[-- Type: text/plain, Size: 7844 bytes --]

x86/shadow: drop stray name tags from sh_{guest_get,map}_eff_l1e()

They (as a now being removed comment validly says) depend only on Xen's
number of page table levels, and hence their tags didn't serve any
useful purpose (there could only ever be one instance in a single
binary, even back in the x86-32 days).

Further conditionalize the inclusion of PV-specific hook pointers, at
once making sure that PV guests can't ever get other than 4-level mode
enabled for them.

For consistency reasons shadow_{write,cmpxchg}_guest_entry() also get
moved next to the other PV-only actors, allowing them to become static
just like the $subject ones do.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -84,7 +84,9 @@ void shadow_vcpu_init(struct vcpu *v)
     }
 #endif
 
-    v->arch.paging.mode = &SHADOW_INTERNAL_NAME(sh_paging_mode, 3);
+    v->arch.paging.mode = is_pv_vcpu(v) ?
+                          &SHADOW_INTERNAL_NAME(sh_paging_mode, 4) :
+                          &SHADOW_INTERNAL_NAME(sh_paging_mode, 3);
 }
 
 #if SHADOW_AUDIT
@@ -1128,38 +1130,6 @@ sh_validate_guest_pt_write(struct vcpu *
     }
 }
 
-int shadow_write_guest_entry(struct vcpu *v, intpte_t *p,
-                             intpte_t new, mfn_t gmfn)
-/* Write a new value into the guest pagetable, and update the shadows
- * appropriately.  Returns 0 if we page-faulted, 1 for success. */
-{
-    int failed;
-    paging_lock(v->domain);
-    failed = __copy_to_user(p, &new, sizeof(new));
-    if ( failed != sizeof(new) )
-        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
-    paging_unlock(v->domain);
-    return (failed == 0);
-}
-
-int shadow_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p,
-                               intpte_t *old, intpte_t new, mfn_t gmfn)
-/* Cmpxchg a new value into the guest pagetable, and update the shadows
- * appropriately. Returns 0 if we page-faulted, 1 if not.
- * N.B. caller should check the value of "old" to see if the
- * cmpxchg itself was successful. */
-{
-    int failed;
-    intpte_t t = *old;
-    paging_lock(v->domain);
-    failed = cmpxchg_user(p, t, new);
-    if ( t == *old )
-        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
-    *old = t;
-    paging_unlock(v->domain);
-    return (failed == 0);
-}
-
 
 /**************************************************************************/
 /* Memory management for shadow pages. */
@@ -2782,14 +2752,7 @@ static void sh_update_paging_modes(struc
     if ( v->arch.paging.mode )
         v->arch.paging.mode->shadow.detach_old_tables(v);
 
-    if ( is_pv_domain(d) )
-    {
-        ///
-        /// PV guest
-        ///
-        v->arch.paging.mode = &SHADOW_INTERNAL_NAME(sh_paging_mode, 4);
-    }
-    else
+    if ( !is_pv_domain(d) )
     {
         ///
         /// HVM guest
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -371,7 +371,7 @@ static void sh_audit_gw(struct vcpu *v, 
 
 
 #if (CONFIG_PAGING_LEVELS == GUEST_PAGING_LEVELS)
-void *
+static void *
 sh_guest_map_l1e(struct vcpu *v, unsigned long addr,
                   unsigned long *gl1mfn)
 {
@@ -395,7 +395,7 @@ sh_guest_map_l1e(struct vcpu *v, unsigne
     return pl1e;
 }
 
-void
+static void
 sh_guest_get_eff_l1e(struct vcpu *v, unsigned long addr, void *eff_l1e)
 {
     walk_t gw;
@@ -408,6 +408,48 @@ sh_guest_get_eff_l1e(struct vcpu *v, uns
     (void) sh_walk_guest_tables(v, addr, &gw, PFEC_page_present);
     *(guest_l1e_t *)eff_l1e = gw.l1e;
 }
+
+/*
+ * Write a new value into the guest pagetable, and update the shadows
+ * appropriately.  Returns 0 if we page-faulted, 1 for success.
+ */
+static int
+sh_write_guest_entry(struct vcpu *v, guest_intpte_t *p,
+                     guest_intpte_t new, mfn_t gmfn)
+{
+    int failed;
+
+    paging_lock(v->domain);
+    failed = __copy_to_user(p, &new, sizeof(new));
+    if ( failed != sizeof(new) )
+        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
+    paging_unlock(v->domain);
+
+    return !failed;
+}
+
+/*
+ * Cmpxchg a new value into the guest pagetable, and update the shadows
+ * appropriately. Returns 0 if we page-faulted, 1 if not.
+ * N.B. caller should check the value of "old" to see if the cmpxchg itself
+ * was successful.
+ */
+static int
+sh_cmpxchg_guest_entry(struct vcpu *v, guest_intpte_t *p,
+                       guest_intpte_t *old, guest_intpte_t new, mfn_t gmfn)
+{
+    int failed;
+    guest_intpte_t t = *old;
+
+    paging_lock(v->domain);
+    failed = cmpxchg_user(p, t, new);
+    if ( t == *old )
+        sh_validate_guest_entry(v, gmfn, p, sizeof(new));
+    *old = t;
+    paging_unlock(v->domain);
+
+    return !failed;
+}
 #endif /* CONFIG == GUEST (== SHADOW) */
 
 /**************************************************************************/
@@ -5184,10 +5226,12 @@ const struct paging_mode sh_paging_mode 
     .update_cr3                    = sh_update_cr3,
     .update_paging_modes           = shadow_update_paging_modes,
     .write_p2m_entry               = shadow_write_p2m_entry,
-    .write_guest_entry             = shadow_write_guest_entry,
-    .cmpxchg_guest_entry           = shadow_cmpxchg_guest_entry,
+#if CONFIG_PAGING_LEVELS == GUEST_PAGING_LEVELS
+    .write_guest_entry             = sh_write_guest_entry,
+    .cmpxchg_guest_entry           = sh_cmpxchg_guest_entry,
     .guest_map_l1e                 = sh_guest_map_l1e,
     .guest_get_eff_l1e             = sh_guest_get_eff_l1e,
+#endif
     .guest_levels                  = GUEST_PAGING_LEVELS,
     .shadow.detach_old_tables      = sh_detach_old_tables,
     .shadow.x86_emulate_write      = sh_x86_emulate_write,
--- a/xen/arch/x86/mm/shadow/multi.h
+++ b/xen/arch/x86/mm/shadow/multi.h
@@ -98,13 +98,6 @@ SHADOW_INTERNAL_NAME(sh_audit_l4_table, 
     (struct vcpu *v, mfn_t sl4mfn, mfn_t x);
 #endif
 
-extern void *
-SHADOW_INTERNAL_NAME(sh_guest_map_l1e, GUEST_LEVELS)
-    (struct vcpu *v, unsigned long va, unsigned long *gl1mfn);
-extern void
-SHADOW_INTERNAL_NAME(sh_guest_get_eff_l1e, GUEST_LEVELS)
-    (struct vcpu *v, unsigned long va, void *eff_l1e);
-
 extern const struct paging_mode
 SHADOW_INTERNAL_NAME(sh_paging_mode, GUEST_LEVELS);
 
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -383,10 +383,6 @@ extern int sh_remove_write_access(struct
 void shadow_write_p2m_entry(struct domain *d, unsigned long gfn,
                             l1_pgentry_t *p, l1_pgentry_t new,
                             unsigned int level);
-int shadow_write_guest_entry(struct vcpu *v, intpte_t *p,
-                             intpte_t new, mfn_t gmfn);
-int shadow_cmpxchg_guest_entry(struct vcpu *v, intpte_t *p,
-                               intpte_t *old, intpte_t new, mfn_t gmfn);
 
 /* Update all the things that are derived from the guest's CR0/CR3/CR4.
  * Called to initialize paging structures if the paging mode
--- a/xen/arch/x86/mm/shadow/types.h
+++ b/xen/arch/x86/mm/shadow/types.h
@@ -262,12 +262,6 @@ static inline shadow_l4e_t shadow_l4e_fr
 #define sh_rm_write_access_from_sl1p INTERNAL_NAME(sh_rm_write_access_from_sl1p)
 #endif
 
-/* The sh_guest_(map|get)_* functions depends on Xen's paging levels */
-#define sh_guest_map_l1e \
-        SHADOW_INTERNAL_NAME(sh_guest_map_l1e, CONFIG_PAGING_LEVELS)
-#define sh_guest_get_eff_l1e \
-        SHADOW_INTERNAL_NAME(sh_guest_get_eff_l1e, CONFIG_PAGING_LEVELS)
-
 /* sh_make_monitor_table depends only on the number of shadow levels */
 #define sh_make_monitor_table \
         SHADOW_SH_NAME(sh_make_monitor_table, SHADOW_PAGING_LEVELS)

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH 09/10] x86/shadow: drop stray name tags from sh_{guest_get, map}_eff_l1e()

[Tim Deegan]

At 04:45 -0600 on 20 Oct (1445316306), Jan Beulich wrote:
> They (as a now being removed comment validly says) depend only on Xen's
> number of page table levels, and hence their tags didn't serve any
> useful purpose (there could only ever be one instance in a single
> binary, even back in the x86-32 days).
> 
> Further conditionalize the inclusion of PV-specific hook pointers, at
> once making sure that PV guests can't ever get other than 4-level mode
> enabled for them.
> 
> For consistency reasons shadow_{write,cmpxchg}_guest_entry() also get
> moved next to the other PV-only actors, allowing them to become static
> just like the $subject ones do.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Tim Deegan <tim@xen.org>

[PATCH RFC 10/10] memory/compat: rename get_reserved_device_memory()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

... to distinguish it from its non-compat counterpart.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Perhaps replace by a more generic patch adding .file to all compat/*.c files?

--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -23,8 +23,7 @@ struct get_reserved_device_memory {
     unsigned int used_entries;
 };
 
-static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
-                                      u32 id, void *ctxt)
+static int compat_grdm(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
 {
     struct get_reserved_device_memory *grdm = ctxt;
     u32 sbdf = PCI_SBDF3(grdm->map.dev.pci.seg, grdm->map.dev.pci.bus,
@@ -355,8 +354,7 @@ int compat_memory_op(unsigned int cmd, X
                 return -EINVAL;
 
             grdm.used_entries = 0;
-            rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
-                                                  &grdm);
+            rc = iommu_get_reserved_device_memory(compat_grdm, &grdm);
 
             if ( !rc && grdm.map.nr_entries < grdm.used_entries )
                 rc = -ENOBUFS;




[-- Attachment #2: grdm-unique.patch --]
[-- Type: text/plain, Size: 1210 bytes --]

memory/compat: rename get_reserved_device_memory()

... to distinguish it from its non-compat counterpart.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Perhaps replace by a more generic patch adding .file to all compat/*.c files?

--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -23,8 +23,7 @@ struct get_reserved_device_memory {
     unsigned int used_entries;
 };
 
-static int get_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr,
-                                      u32 id, void *ctxt)
+static int compat_grdm(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
 {
     struct get_reserved_device_memory *grdm = ctxt;
     u32 sbdf = PCI_SBDF3(grdm->map.dev.pci.seg, grdm->map.dev.pci.bus,
@@ -355,8 +354,7 @@ int compat_memory_op(unsigned int cmd, X
                 return -EINVAL;
 
             grdm.used_entries = 0;
-            rc = iommu_get_reserved_device_memory(get_reserved_device_memory,
-                                                  &grdm);
+            rc = iommu_get_reserved_device_memory(compat_grdm, &grdm);
 
             if ( !rc && grdm.map.nr_entries < grdm.used_entries )
                 rc = -ENOBUFS;

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel