[PATCH v2 0/5] disambiguate symbol names (part 2)

[PATCH v2 0/5] disambiguate symbol names (part 2)

[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
revised remainder 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 (most of which already went in). In
other cases renaming is necessary. And then there are possibly -
compiler dependent - a few collisions left in place by the end of this
series, but in v2 it's just one that I actively observed (in cpufreq.c).

01: symbols: prefix static symbols with their source file names
02: compat: enforce distinguishable file names in symbol table
03: x86/mm: override stored file names for multiply built sources
04: x86/mm: build map_domain_gfn() just once
05: x86/mm: only a single instance of gw_page_flags[] is needed

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

[PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 9457 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>
---
v2: Also special case file names with directory part (along with
    object ones). Mirror xen-syms linking changes to ARM, but for now
    without generating warnings.

--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -80,11 +80,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 >$(@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 $@
--- 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, single_source, multi_source } 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,31 @@ 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
+		 * or has a directory component (to cover multiply compiled
+		 * files).
+		 */
+		bool multi = strchr(str, '/') || (sym && sym[1] == 'o');
+
+		if (multi || last != multi_source) {
+			free(filename);
+			filename = *str ? strdup(str) : NULL;
+		}
+		last = multi ? multi_source : single_source;
+		goto skip_tail;
+	}
+
+	last = symbol;
+	rc = -1;
+
 	sym = str;
 	/* skip prefix char */
 	if (symbol_prefix_char && str[0] == symbol_prefix_char)
@@ -109,24 +150,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 +514,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 +554,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: 9516 bytes --]

symbols: prefix static symbols with their source file names

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>
---
v2: Also special case file names with directory part (along with
    object ones). Mirror xen-syms linking changes to ARM, but for now
    without generating warnings.

--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -80,11 +80,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 >$(@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 $@
--- 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, single_source, multi_source } 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,31 @@ 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
+		 * or has a directory component (to cover multiply compiled
+		 * files).
+		 */
+		bool multi = strchr(str, '/') || (sym && sym[1] == 'o');
+
+		if (multi || last != multi_source) {
+			free(filename);
+			filename = *str ? strdup(str) : NULL;
+		}
+		last = multi ? multi_source : single_source;
+		goto skip_tail;
+	}
+
+	last = symbol;
+	rc = -1;
+
 	sym = str;
 	/* skip prefix char */
 	if (symbol_prefix_char && str[0] == symbol_prefix_char)
@@ -109,24 +150,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 +514,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 +554,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 v2 1/5] symbols: prefix static symbols with their source file names

[Andrew Cooper]

On 26/10/15 11:49, 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>

Should warn_dups become fatal once the patches to fix these...

Duplicate symbol 'memory.c#get_reserved_device_memory' (ffff82d080140183
!= ffff82d080118b5b)
Duplicate symbol 'platform_hypercall.c#__maddr_to_virt'
(ffff82d08023a3a2 != ffff82d080167e66)
Duplicate symbol 'platform_hypercall.c#__virt_to_maddr'
(ffff82d08023a401 != ffff82d080167ec5)
Duplicate symbol 'platform_hypercall.c#cpumask_check' (ffff82d08023a489
!= ffff82d080167f4d)

have been committed, to avoid accidental reintroduction?

I note that even sysv format doesn't appear to provide directory
information, so we still cant distinguish duplicate static symbols in
identically named source files in difference directories, but hopefully
that should be very rare.

Overall, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>,
although I haven’t vetted the symbol.c changes too carefully.

~Andrew

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Jan Beulich]

>>> On 28.10.15 at 13:55, <andrew.cooper3@citrix.com> wrote:
> On 26/10/15 11:49, 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>
> 
> Should warn_dups become fatal once the patches to fix these...
> 
> Duplicate symbol 'memory.c#get_reserved_device_memory' (ffff82d080140183
> != ffff82d080118b5b)
> Duplicate symbol 'platform_hypercall.c#__maddr_to_virt'
> (ffff82d08023a3a2 != ffff82d080167e66)
> Duplicate symbol 'platform_hypercall.c#__virt_to_maddr'
> (ffff82d08023a401 != ffff82d080167ec5)
> Duplicate symbol 'platform_hypercall.c#cpumask_check' (ffff82d08023a489
> != ffff82d080167f4d)
> 
> have been committed, to avoid accidental reintroduction?

They all went in already. Or are you saying you saw these on top
of what is in staging right now?

However - no to the question here. For one, there's nothing fatal
about it the absence of xSplice. And even with xSplice I'm not sure
this really should be fatal at the build stage. What should force
people to at least look closely would be a runtime patch using such
a symbol. And second, ...

> I note that even sysv format doesn't appear to provide directory
> information, so we still cant distinguish duplicate static symbols in
> identically named source files in difference directories, but hopefully
> that should be very rare.

... this. I actually see one with some gcc versions (an inline function
not expanded inline in two different cpufreq.c).

> Overall, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>,
> although I haven’t vetted the symbol.c changes too carefully.

Thanks.

Jan

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

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Andrew Cooper]

On 28/10/15 13:25, Jan Beulich wrote:
>>>> On 28.10.15 at 13:55, <andrew.cooper3@citrix.com> wrote:
>> On 26/10/15 11:49, 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>
>> Should warn_dups become fatal once the patches to fix these...
>>
>> Duplicate symbol 'memory.c#get_reserved_device_memory' (ffff82d080140183
>> != ffff82d080118b5b)
>> Duplicate symbol 'platform_hypercall.c#__maddr_to_virt'
>> (ffff82d08023a3a2 != ffff82d080167e66)
>> Duplicate symbol 'platform_hypercall.c#__virt_to_maddr'
>> (ffff82d08023a401 != ffff82d080167ec5)
>> Duplicate symbol 'platform_hypercall.c#cpumask_check' (ffff82d08023a489
>> != ffff82d080167f4d)
>>
>> have been committed, to avoid accidental reintroduction?
> They all went in already. Or are you saying you saw these on top
> of what is in staging right now?

Current staging right now

andrewcoop@andrewcoop:/local/xen.git/xen$ git log --oneline staging^..
69bdd7f symbols: prefix static symbols with their source file names
bf0d492 x86/mm: don't call HVM-only function for PV guests

However, those duplicates are from the compat code, which I didn't
specifically take your patch 2 for.

>
> However - no to the question here. For one, there's nothing fatal
> about it the absence of xSplice. And even with xSplice I'm not sure
> this really should be fatal at the build stage.

For the non-xSplice case, the worst which can happen is indeed just a
harder-to-read stack trace.

However, for the xSplice case, we really should take reasonable steps to
make patching easier, and that includes avoiding duplicate symbols.

As a future user of xSplice, I would definitely like an option to fail
the hypervisor build if it will result in a hard-to-patch binary.

>  What should force
> people to at least look closely would be a runtime patch using such
> a symbol. And second, ...
>
>> I note that even sysv format doesn't appear to provide directory
>> information, so we still cant distinguish duplicate static symbols in
>> identically named source files in difference directories, but hopefully
>> that should be very rare.
> ... this. I actually see one with some gcc versions (an inline function
> not expanded inline in two different cpufreq.c).

An inline function with an ASSERT/BUG or alternative by any chance?  GCC
appears to aggressively out-of-line these, which is why had to sprinkle
always_inline to helpers such as stac()/clac()

~Andrew

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Jan Beulich]

>>> On 28.10.15 at 14:42, <andrew.cooper3@citrix.com> wrote:
> On 28/10/15 13:25, Jan Beulich wrote:
>>>>> On 28.10.15 at 13:55, <andrew.cooper3@citrix.com> wrote:
>>> On 26/10/15 11:49, 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>
>>> Should warn_dups become fatal once the patches to fix these...
>>>
>>> Duplicate symbol 'memory.c#get_reserved_device_memory' (ffff82d080140183
>>> != ffff82d080118b5b)
>>> Duplicate symbol 'platform_hypercall.c#__maddr_to_virt'
>>> (ffff82d08023a3a2 != ffff82d080167e66)
>>> Duplicate symbol 'platform_hypercall.c#__virt_to_maddr'
>>> (ffff82d08023a401 != ffff82d080167ec5)
>>> Duplicate symbol 'platform_hypercall.c#cpumask_check' (ffff82d08023a489
>>> != ffff82d080167f4d)
>>>
>>> have been committed, to avoid accidental reintroduction?
>> They all went in already. Or are you saying you saw these on top
>> of what is in staging right now?
> 
> Current staging right now
> 
> andrewcoop@andrewcoop:/local/xen.git/xen$ git log --oneline staging^..
> 69bdd7f symbols: prefix static symbols with their source file names
> bf0d492 x86/mm: don't call HVM-only function for PV guests
> 
> However, those duplicates are from the compat code, which I didn't
> specifically take your patch 2 for.

Ah, of course - that's what that patch is for.

>>> I note that even sysv format doesn't appear to provide directory
>>> information, so we still cant distinguish duplicate static symbols in
>>> identically named source files in difference directories, but hopefully
>>> that should be very rare.
>> ... this. I actually see one with some gcc versions (an inline function
>> not expanded inline in two different cpufreq.c).
> 
> An inline function with an ASSERT/BUG or alternative by any chance?  GCC
> appears to aggressively out-of-line these, which is why had to sprinkle
> always_inline to helpers such as stac()/clac()

cpumask_first() or one of its relatives.

Jan

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Ian Campbell]

On Mon, 2015-10-26 at 05:49 -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>
> ---
> v2: Also special case file names with directory part (along with
>     object ones). Mirror xen-syms linking changes to ARM, but for now
>     without generating warnings.

Thanks for taking care of ARM. I assume "without generating warnings"
differs from the x86 behaviour? Is there some followup which us ARM folks
ought to be looking into making?

Ian.

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Jan Beulich]

>>> On 02.11.15 at 14:47, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-10-26 at 05:49 -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>
>> ---
>> v2: Also special case file names with directory part (along with
>>     object ones). Mirror xen-syms linking changes to ARM, but for now
>>     without generating warnings.
> 
> Thanks for taking care of ARM. I assume "without generating warnings"
> differs from the x86 behaviour? Is there some followup which us ARM folks
> ought to be looking into making?

Not right now I would say. As mentioned elsewhere, without
xSplice as a goal I don't see much use in the having those warnings.

Jan

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Ian Campbell]

On Mon, 2015-11-02 at 06:55 -0700, Jan Beulich wrote:
> > > > On 02.11.15 at 14:47, <ian.campbell@citrix.com> wrote:
> > On Mon, 2015-10-26 at 05:49 -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>
> > > ---
> > > v2: Also special case file names with directory part (along with
> > >     object ones). Mirror xen-syms linking changes to ARM, but for now
> > >     without generating warnings.
> > 
> > Thanks for taking care of ARM. I assume "without generating warnings"
> > differs from the x86 behaviour? Is there some followup which us ARM
> > folks
> > ought to be looking into making?
> 
> Not right now I would say. As mentioned elsewhere, without
> xSplice as a goal I don't see much use in the having those warnings.

OK, thanks.

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Jan Beulich]

>>> On 02.11.15 at 15:54, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-11-02 at 06:55 -0700, Jan Beulich wrote:
>> > > > On 02.11.15 at 14:47, <ian.campbell@citrix.com> wrote:
>> > On Mon, 2015-10-26 at 05:49 -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>
>> > > ---
>> > > v2: Also special case file names with directory part (along with
>> > >     object ones). Mirror xen-syms linking changes to ARM, but for now
>> > >     without generating warnings.
>> > 
>> > Thanks for taking care of ARM. I assume "without generating warnings"
>> > differs from the x86 behaviour? Is there some followup which us ARM
>> > folks
>> > ought to be looking into making?
>> 
>> Not right now I would say. As mentioned elsewhere, without
>> xSplice as a goal I don't see much use in the having those warnings.
> 
> OK, thanks.

So since you're on it - can I get an ack (or otherwise)? Perhaps
also on patch 2?

Thanks, Jan

Re: [PATCH v2 1/5] symbols: prefix static symbols with their source file names

[Ian Campbell]

On Mon, 2015-11-02 at 07:58 -0700, Jan Beulich wrote:

> So since you're on it - can I get an ack (or otherwise)? Perhaps
> also on patch 2?

That being "compat: enforce distinguishable file names in symbol table" ?

TBH I had considered that to be x86 specific (despite where the code might
live). I'll go take a look now though.

Ian.

[PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Jan Beulich]

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

To make it possible to tell apart the static symbols in files built a
second for compat guest support, arrange for their source file names to
be prefixed by a suitable path. We can't do this without explicit .file
directives, since gcc has always been stripping paths from file names
handed to the internally generated .file directive. However, we can
leverage __FILE__ if we make sure the second instance gets compiled out
of other than the very directory the wrapper sits in.

Where suitable, remove the long redundant explicit inclusions of
xen/config.h at once.

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

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -86,8 +86,7 @@ AFLAGS-$(clang)         += -no-integrate
 ALL_OBJS := $(ALL_OBJS-y)
 
 # Get gcc to generate the dependencies for us.
-CFLAGS-y += -MMD -MF .$(@F).d
-DEPS = .*.d
+CFLAGS-y += -MMD -MF $(@D)/.$(@F).d
 
 CFLAGS += $(CFLAGS-y)
 
@@ -103,6 +102,14 @@ LDFLAGS += $(LDFLAGS-y)
 
 include Makefile
 
+DEPS = .*.d
+define gendep
+    ifneq ($(1),$(subst /,:,$(1)))
+        DEPS += $(dir $(1)).$(basename $(notdir $(1))).d
+    endif
+endef
+$(foreach o,$(filter-out %/,$(obj-y)),$(eval $(call gendep,$(o))))
+
 # Ensure each subdirectory has exactly one trailing slash.
 subdir-n := $(patsubst %,%/,$(patsubst %/,%,$(subdir-n) $(subdir-)))
 subdir-y := $(patsubst %,%/,$(patsubst %/,%,$(subdir-y)))
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -13,7 +13,7 @@ obj-y += bitops.o
 obj-bin-y += bzimage.init.o
 obj-bin-y += clear_page.o
 obj-bin-y += copy_page.o
-obj-y += compat.o
+obj-y += compat.o x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
 obj-y += debug.o
 obj-y += delay.o
@@ -25,7 +25,6 @@ obj-y += domain_page.o
 obj-y += e820.o
 obj-y += extable.o
 obj-y += flushtlb.o
-obj-y += platform_hypercall.o
 obj-y += i387.o
 obj-y += i8259.o
 obj-y += io_apic.o
@@ -37,14 +36,15 @@ obj-y += microcode_amd.o
 obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
-obj-y += mm.o
+obj-y += mm.o x86_64/mm.o
 obj-y += monitor.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
 obj-y += pci.o
 obj-y += percpu.o
-obj-y += physdev.o
+obj-y += physdev.o x86_64/physdev.o
+obj-y += platform_hypercall.o x86_64/platform_hypercall.o
 obj-y += psr.o
 obj-y += setup.o
 obj-y += shutdown.o
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -2,7 +2,6 @@ subdir-y += compat
 
 obj-bin-y += entry.o
 obj-bin-y += gpr_switch.o
-obj-y += mm.o
 obj-y += traps.o
 obj-y += machine_kexec.o
 obj-y += pci.o
@@ -10,10 +9,7 @@ obj-y += acpi_mmcfg.o
 obj-y += mmconf-fam10h.o
 obj-y += mmconfig_64.o
 obj-y += mmconfig-shared.o
-obj-y += compat.o
 obj-y += domain.o
-obj-y += physdev.o
-obj-y += platform_hypercall.o
 obj-y += cpu_idle.o
 obj-y += cpufreq.o
 obj-bin-y += kexec_reloc.o
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -2,7 +2,8 @@
  * compat.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/hypercall.h>
 #include <compat/xen.h>
 #include <compat/physdev.h>
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -16,7 +16,8 @@
  * with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/lib.h>
 #include <xen/init.h>
 #include <xen/mm.h>
--- a/xen/arch/x86/x86_64/physdev.c
+++ b/xen/arch/x86/x86_64/physdev.c
@@ -2,7 +2,8 @@
  * physdev.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/guest_access.h>
 #include <compat/xen.h>
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -2,7 +2,8 @@
  * platform_hypercall.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/lib.h>
 #include <compat/platform.h>
 
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -63,7 +63,7 @@ obj-$(perfc)       += perfc.o
 obj-$(crash_debug) += gdbstub.o
 obj-$(xenoprof)    += xenoprof.o
 
-subdir-$(CONFIG_COMPAT) += compat
+obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o)
 
 subdir-$(x86_64) += hvm
 
--- a/xen/common/compat/Makefile
+++ /dev/null
@@ -1,6 +0,0 @@
-obj-y += domain.o
-obj-y += kernel.o
-obj-y += memory.o
-obj-y += multicall.o
-obj-y += xlat.o
-obj-y += tmem_xen.o
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -3,7 +3,8 @@
  *
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/domain.h>
--- a/xen/common/compat/kernel.c
+++ b/xen/common/compat/kernel.c
@@ -2,7 +2,8 @@
  * kernel.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/errno.h>
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -1,4 +1,5 @@
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/hypercall.h>
 #include <xen/guest_access.h>
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -2,7 +2,8 @@
  * multicall.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/multicall.h>
 #include <xen/trace.h>
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -58,7 +58,7 @@ compat/%.h: compat/%.i Makefile $(BASEDI
 	mv -f $@.new $@
 
 compat/%.i: compat/%.c Makefile
-	$(CPP) $(filter-out -M% .%.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
+	$(CPP) $(filter-out -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
 
 compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py
 	mkdir -p $(@D)



[-- Attachment #2: compat-file-names.patch --]
[-- Type: text/plain, Size: 6030 bytes --]

compat: enforce distinguishable file names in symbol table

To make it possible to tell apart the static symbols in files built a
second for compat guest support, arrange for their source file names to
be prefixed by a suitable path. We can't do this without explicit .file
directives, since gcc has always been stripping paths from file names
handed to the internally generated .file directive. However, we can
leverage __FILE__ if we make sure the second instance gets compiled out
of other than the very directory the wrapper sits in.

Where suitable, remove the long redundant explicit inclusions of
xen/config.h at once.

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

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -86,8 +86,7 @@ AFLAGS-$(clang)         += -no-integrate
 ALL_OBJS := $(ALL_OBJS-y)
 
 # Get gcc to generate the dependencies for us.
-CFLAGS-y += -MMD -MF .$(@F).d
-DEPS = .*.d
+CFLAGS-y += -MMD -MF $(@D)/.$(@F).d
 
 CFLAGS += $(CFLAGS-y)
 
@@ -103,6 +102,14 @@ LDFLAGS += $(LDFLAGS-y)
 
 include Makefile
 
+DEPS = .*.d
+define gendep
+    ifneq ($(1),$(subst /,:,$(1)))
+        DEPS += $(dir $(1)).$(basename $(notdir $(1))).d
+    endif
+endef
+$(foreach o,$(filter-out %/,$(obj-y)),$(eval $(call gendep,$(o))))
+
 # Ensure each subdirectory has exactly one trailing slash.
 subdir-n := $(patsubst %,%/,$(patsubst %/,%,$(subdir-n) $(subdir-)))
 subdir-y := $(patsubst %,%/,$(patsubst %/,%,$(subdir-y)))
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -13,7 +13,7 @@ obj-y += bitops.o
 obj-bin-y += bzimage.init.o
 obj-bin-y += clear_page.o
 obj-bin-y += copy_page.o
-obj-y += compat.o
+obj-y += compat.o x86_64/compat.o
 obj-$(CONFIG_KEXEC) += crash.o
 obj-y += debug.o
 obj-y += delay.o
@@ -25,7 +25,6 @@ obj-y += domain_page.o
 obj-y += e820.o
 obj-y += extable.o
 obj-y += flushtlb.o
-obj-y += platform_hypercall.o
 obj-y += i387.o
 obj-y += i8259.o
 obj-y += io_apic.o
@@ -37,14 +36,15 @@ obj-y += microcode_amd.o
 obj-y += microcode_intel.o
 # This must come after the vendor specific files.
 obj-y += microcode.o
-obj-y += mm.o
+obj-y += mm.o x86_64/mm.o
 obj-y += monitor.o
 obj-y += mpparse.o
 obj-y += nmi.o
 obj-y += numa.o
 obj-y += pci.o
 obj-y += percpu.o
-obj-y += physdev.o
+obj-y += physdev.o x86_64/physdev.o
+obj-y += platform_hypercall.o x86_64/platform_hypercall.o
 obj-y += psr.o
 obj-y += setup.o
 obj-y += shutdown.o
--- a/xen/arch/x86/x86_64/Makefile
+++ b/xen/arch/x86/x86_64/Makefile
@@ -2,7 +2,6 @@ subdir-y += compat
 
 obj-bin-y += entry.o
 obj-bin-y += gpr_switch.o
-obj-y += mm.o
 obj-y += traps.o
 obj-y += machine_kexec.o
 obj-y += pci.o
@@ -10,10 +9,7 @@ obj-y += acpi_mmcfg.o
 obj-y += mmconf-fam10h.o
 obj-y += mmconfig_64.o
 obj-y += mmconfig-shared.o
-obj-y += compat.o
 obj-y += domain.o
-obj-y += physdev.o
-obj-y += platform_hypercall.o
 obj-y += cpu_idle.o
 obj-y += cpufreq.o
 obj-bin-y += kexec_reloc.o
--- a/xen/arch/x86/x86_64/compat.c
+++ b/xen/arch/x86/x86_64/compat.c
@@ -2,7 +2,8 @@
  * compat.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/hypercall.h>
 #include <compat/xen.h>
 #include <compat/physdev.h>
--- a/xen/arch/x86/x86_64/mm.c
+++ b/xen/arch/x86/x86_64/mm.c
@@ -16,7 +16,8 @@
  * with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/lib.h>
 #include <xen/init.h>
 #include <xen/mm.h>
--- a/xen/arch/x86/x86_64/physdev.c
+++ b/xen/arch/x86/x86_64/physdev.c
@@ -2,7 +2,8 @@
  * physdev.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/guest_access.h>
 #include <compat/xen.h>
--- a/xen/arch/x86/x86_64/platform_hypercall.c
+++ b/xen/arch/x86/x86_64/platform_hypercall.c
@@ -2,7 +2,8 @@
  * platform_hypercall.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/lib.h>
 #include <compat/platform.h>
 
--- a/xen/common/Makefile
+++ b/xen/common/Makefile
@@ -63,7 +63,7 @@ obj-$(perfc)       += perfc.o
 obj-$(crash_debug) += gdbstub.o
 obj-$(xenoprof)    += xenoprof.o
 
-subdir-$(CONFIG_COMPAT) += compat
+obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o multicall.o tmem_xen.o xlat.o)
 
 subdir-$(x86_64) += hvm
 
--- a/xen/common/compat/Makefile
+++ /dev/null
@@ -1,6 +0,0 @@
-obj-y += domain.o
-obj-y += kernel.o
-obj-y += memory.o
-obj-y += multicall.o
-obj-y += xlat.o
-obj-y += tmem_xen.o
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -3,7 +3,8 @@
  *
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/lib.h>
 #include <xen/sched.h>
 #include <xen/domain.h>
--- a/xen/common/compat/kernel.c
+++ b/xen/common/compat/kernel.c
@@ -2,7 +2,8 @@
  * kernel.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/errno.h>
--- a/xen/common/compat/memory.c
+++ b/xen/common/compat/memory.c
@@ -1,4 +1,5 @@
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/hypercall.h>
 #include <xen/guest_access.h>
--- a/xen/common/compat/multicall.c
+++ b/xen/common/compat/multicall.c
@@ -2,7 +2,8 @@
  * multicall.c
  */
 
-#include <xen/config.h>
+asm(".file \"" __FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/multicall.h>
 #include <xen/trace.h>
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -58,7 +58,7 @@ compat/%.h: compat/%.i Makefile $(BASEDI
 	mv -f $@.new $@
 
 compat/%.i: compat/%.c Makefile
-	$(CPP) $(filter-out -M% .%.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
+	$(CPP) $(filter-out -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
 
 compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py
 	mkdir -p $(@D)

[-- 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 v2 2/5] compat: enforce distinguishable file names in symbol table

[Andrew Cooper]

On 26/10/15 11:50, Jan Beulich wrote:
> To make it possible to tell apart the static symbols in files built a
> second for compat guest support, arrange for their source file names to
> be prefixed by a suitable path. We can't do this without explicit .file
> directives, since gcc has always been stripping paths from file names
> handed to the internally generated .file directive. However, we can
> leverage __FILE__ if we make sure the second instance gets compiled out
> of other than the very directory the wrapper sits in.
>
> Where suitable, remove the long redundant explicit inclusions of
> xen/config.h at once.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Ian Campbell]

On Mon, 2015-10-26 at 05:50 -0600, Jan Beulich wrote:
> To make it possible to tell apart the static symbols in files built a
> second for compat guest support, arrange for their source file names to

        ^ time ?

> be prefixed by a suitable path. We can't do this without explicit .file
> directives, since gcc has always been stripping paths from file names
> handed to the internally generated .file directive. However, we can
> leverage __FILE__ if we make sure the second instance gets compiled out
> of other than the very directory the wrapper sits in.
> 
> Where suitable, remove the long redundant explicit inclusions of
> xen/config.h at once.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -86,8 +86,7 @@ AFLAGS-$(clang)         += -no-integrate
>  ALL_OBJS := $(ALL_OBJS-y)
>  
>  # Get gcc to generate the dependencies for us.
> -CFLAGS-y += -MMD -MF .$(@F).d
> -DEPS = .*.d
> +CFLAGS-y += -MMD -MF $(@D)/.$(@F).d
>  
>  CFLAGS += $(CFLAGS-y)
>  
> @@ -103,6 +102,14 @@ LDFLAGS += $(LDFLAGS-y)
>  
>  include Makefile
>  
> +DEPS = .*.d
> +define gendep
> +    ifneq ($(1),$(subst /,:,$(1)))
> +        DEPS += $(dir $(1)).$(basename $(notdir $(1))).d
> +    endif
> +endef
> +$(foreach o,$(filter-out %/,$(obj-y)),$(eval $(call gendep,$(o))))

Is this generating a .subdir.file.d for each subdir/file.o in obj-y?

This is as a consequence of now building subdir/file.o from the parent
directory instead of recursing for some subset of files?

It seems quite inconsistent to me to have xen/arch/x86/x86_64/Makefile
building some files directly and xen/arch/x86/Makefile to be building
another subset of those files via x86_64/FOO.o. Even more so that other
than compat.o I can't see what makes many affected files (e.g. mm.o or
platform_hypercall.o) special in this regard.

Does all of that fall out from a desire to reuse __FILE__? If so I'm
inclined to suggest that -DBUILD_FILENAME_PREFIX="compat/" or whatever
would seem likely to me to end up less strange (but maybe you tried that
and it was worse?).

> +
>  # Ensure each subdirectory has exactly one trailing slash.
>  subdir-n := $(patsubst %,%/,$(patsubst %/,%,$(subdir-n) $(subdir-)))
>  subdir-y := $(patsubst %,%/,$(patsubst %/,%,$(subdir-y)))
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -13,7 +13,7 @@ obj-y += bitops.o
>  obj-bin-y += bzimage.init.o
>  obj-bin-y += clear_page.o
>  obj-bin-y += copy_page.o
> -obj-y += compat.o
> +obj-y += compat.o x86_64/compat.o
>  obj-$(CONFIG_KEXEC) += crash.o
>  obj-y += debug.o
>  obj-y += delay.o
> @@ -25,7 +25,6 @@ obj-y += domain_page.o
>  obj-y += e820.o
>  obj-y += extable.o
>  obj-y += flushtlb.o
> -obj-y += platform_hypercall.o
>  obj-y += i387.o
>  obj-y += i8259.o
>  obj-y += io_apic.o
> @@ -37,14 +36,15 @@ obj-y += microcode_amd.o
>  obj-y += microcode_intel.o
>  # This must come after the vendor specific files.
>  obj-y += microcode.o
> -obj-y += mm.o
> +obj-y += mm.o x86_64/mm.o
>  obj-y += monitor.o
>  obj-y += mpparse.o
>  obj-y += nmi.o
>  obj-y += numa.o
>  obj-y += pci.o
>  obj-y += percpu.o
> -obj-y += physdev.o
> +obj-y += physdev.o x86_64/physdev.o
> +obj-y += platform_hypercall.o x86_64/platform_hypercall.o
>  obj-y += psr.o
>  obj-y += setup.o
>  obj-y += shutdown.o
> --- a/xen/arch/x86/x86_64/Makefile
> +++ b/xen/arch/x86/x86_64/Makefile
> @@ -2,7 +2,6 @@ subdir-y += compat
>  
>  obj-bin-y += entry.o
>  obj-bin-y += gpr_switch.o
> -obj-y += mm.o
>  obj-y += traps.o
>  obj-y += machine_kexec.o
>  obj-y += pci.o
> @@ -10,10 +9,7 @@ obj-y += acpi_mmcfg.o
>  obj-y += mmconf-fam10h.o
>  obj-y += mmconfig_64.o
>  obj-y += mmconfig-shared.o
> -obj-y += compat.o
>  obj-y += domain.o
> -obj-y += physdev.o
> -obj-y += platform_hypercall.o
>  obj-y += cpu_idle.o
>  obj-y += cpufreq.o
>  obj-bin-y += kexec_reloc.o
> --- a/xen/arch/x86/x86_64/compat.c
> +++ b/xen/arch/x86/x86_64/compat.c
> @@ -2,7 +2,8 @@
>   * compat.c
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/hypercall.h>
>  #include <compat/xen.h>
>  #include <compat/physdev.h>
> --- a/xen/arch/x86/x86_64/mm.c
> +++ b/xen/arch/x86/x86_64/mm.c
> @@ -16,7 +16,8 @@
>   * with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/lib.h>
>  #include <xen/init.h>
>  #include <xen/mm.h>
> --- a/xen/arch/x86/x86_64/physdev.c
> +++ b/xen/arch/x86/x86_64/physdev.c
> @@ -2,7 +2,8 @@
>   * physdev.c
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/types.h>
>  #include <xen/guest_access.h>
>  #include <compat/xen.h>
> --- a/xen/arch/x86/x86_64/platform_hypercall.c
> +++ b/xen/arch/x86/x86_64/platform_hypercall.c
> @@ -2,7 +2,8 @@
>   * platform_hypercall.c
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/lib.h>
>  #include <compat/platform.h>
>  
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -63,7 +63,7 @@ obj-$(perfc)       += perfc.o
>  obj-$(crash_debug) += gdbstub.o
>  obj-$(xenoprof)    += xenoprof.o
>  
> -subdir-$(CONFIG_COMPAT) += compat
> +obj-$(CONFIG_COMPAT) += $(addprefix compat/,domain.o kernel.o memory.o
> multicall.o tmem_xen.o xlat.o)
>  
>  subdir-$(x86_64) += hvm
>  
> --- a/xen/common/compat/Makefile
> +++ /dev/null
> @@ -1,6 +0,0 @@
> -obj-y += domain.o
> -obj-y += kernel.o
> -obj-y += memory.o
> -obj-y += multicall.o
> -obj-y += xlat.o
> -obj-y += tmem_xen.o
> --- a/xen/common/compat/domain.c
> +++ b/xen/common/compat/domain.c
> @@ -3,7 +3,8 @@
>   *
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/lib.h>
>  #include <xen/sched.h>
>  #include <xen/domain.h>
> --- a/xen/common/compat/kernel.c
> +++ b/xen/common/compat/kernel.c
> @@ -2,7 +2,8 @@
>   * kernel.c
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/init.h>
>  #include <xen/lib.h>
>  #include <xen/errno.h>
> --- a/xen/common/compat/memory.c
> +++ b/xen/common/compat/memory.c
> @@ -1,4 +1,5 @@
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/types.h>
>  #include <xen/hypercall.h>
>  #include <xen/guest_access.h>
> --- a/xen/common/compat/multicall.c
> +++ b/xen/common/compat/multicall.c
> @@ -2,7 +2,8 @@
>   * multicall.c
>   */
>  
> -#include <xen/config.h>
> +asm(".file \"" __FILE__ "\"");
> +
>  #include <xen/types.h>
>  #include <xen/multicall.h>
>  #include <xen/trace.h>
> --- a/xen/include/Makefile
> +++ b/xen/include/Makefile
> @@ -58,7 +58,7 @@ compat/%.h: compat/%.i Makefile $(BASEDI
>  	mv -f $@.new $@
>  
>  compat/%.i: compat/%.c Makefile
> -	$(CPP) $(filter-out -M% .%.d -include
> %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
> +	$(CPP) $(filter-out -M% %.d -include
> %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
>  
>  compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build
> -source.py
>  	mkdir -p $(@D)
> 
> 

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Jan Beulich]

>>> On 02.11.15 at 16:20, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-10-26 at 05:50 -0600, Jan Beulich wrote:
>> To make it possible to tell apart the static symbols in files built a
>> second for compat guest support, arrange for their source file names to
> 
>         ^ time ?

Oh, yes, of course.

>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -86,8 +86,7 @@ AFLAGS-$(clang)         += -no-integrate
>>  ALL_OBJS := $(ALL_OBJS-y)
>>  
>>  # Get gcc to generate the dependencies for us.
>> -CFLAGS-y += -MMD -MF .$(@F).d
>> -DEPS = .*.d
>> +CFLAGS-y += -MMD -MF $(@D)/.$(@F).d
>>  
>>  CFLAGS += $(CFLAGS-y)
>>  
>> @@ -103,6 +102,14 @@ LDFLAGS += $(LDFLAGS-y)
>>  
>>  include Makefile
>>  
>> +DEPS = .*.d
>> +define gendep
>> +    ifneq ($(1),$(subst /,:,$(1)))
>> +        DEPS += $(dir $(1)).$(basename $(notdir $(1))).d
>> +    endif
>> +endef
>> +$(foreach o,$(filter-out %/,$(obj-y)),$(eval $(call gendep,$(o))))
> 
> Is this generating a .subdir.file.d for each subdir/file.o in obj-y?

Actually a subdir/.file.d, but other than this minor difference - yes.

> This is as a consequence of now building subdir/file.o from the parent
> directory instead of recursing for some subset of files?

Yes.

> It seems quite inconsistent to me to have xen/arch/x86/x86_64/Makefile
> building some files directly and xen/arch/x86/Makefile to be building
> another subset of those files via x86_64/FOO.o. Even more so that other
> than compat.o I can't see what makes many affected files (e.g. mm.o or
> platform_hypercall.o) special in this regard.

The platform_hypercall one is quite obvious, because
x86_64/platform_hypercall.c just includes platform_hypercall.c
(which is the general pattern we're dealing with here). For mm.c
it was just prompted by actual collisions seen.

> Does all of that fall out from a desire to reuse __FILE__? If so I'm
> inclined to suggest that -DBUILD_FILENAME_PREFIX="compat/" or whatever
> would seem likely to me to end up less strange (but maybe you tried that
> and it was worse?).

Yes to the first question. And no, I didn't try the alternative you
suggest, but discarded it as the uglier variant from the beginning.
In particular I dislike (parts of) file names to be specified on the
command line, rather than getting derived from what we have
anyway.

Considering that Andrew was fine with the x86 parts, I'd want to
change the approach (the x86 side of which I understand is of
particular concern to you) only if you're convinced this alternative
approach is sufficiently much better.

Jan

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Ian Campbell]

On Mon, 2015-11-02 at 09:11 -0700, Jan Beulich wrote:
> > > > 
> > It seems quite inconsistent to me to have xen/arch/x86/x86_64/Makefile
> > building some files directly and xen/arch/x86/Makefile to be building
> > another subset of those files via x86_64/FOO.o. Even more so that other
> > than compat.o I can't see what makes many affected files (e.g. mm.o or
> > platform_hypercall.o) special in this regard.
> 
> The platform_hypercall one is quite obvious, because
> x86_64/platform_hypercall.c just includes platform_hypercall.c
> (which is the general pattern we're dealing with here). For mm.c
> it was just prompted by actual collisions seen.

Neither of those seem obvious to me.

> > Does all of that fall out from a desire to reuse __FILE__? If so I'm
> > inclined to suggest that -DBUILD_FILENAME_PREFIX="compat/" or whatever
> > would seem likely to me to end up less strange (but maybe you tried
> > that
> > and it was worse?).
> 
> Yes to the first question. And no, I didn't try the alternative you
> suggest, but discarded it as the uglier variant from the beginning.
> In particular I dislike (parts of) file names to be specified on the
> command line, rather than getting derived from what we have
> anyway.

Hrm, ok.

What I was actually thinking (but not expressing very well in the example I
gave) was that the code being compiled a second time in "compat" mode would
be tagged as that, e.g. by listing them in obj-compat-y and adding
something to CFLAGS (which may or may not look like a subpath, it might
e.g. just be "(compat)").

Many of the double compiled files end up #defineing COMPAT, even without
moving that to CFLAGS isn't that enough to key such a distinction on.

> Considering that Andrew was fine with the x86 parts, I'd want to
> change the approach (the x86 side of which I understand is of
> particular concern to you) only if you're convinced this alternative
> approach is sufficiently much better.

I'm mostly concerned with precedent being set by x86 and also implied by
the common code Makefile infra which supports it causing people to think it
is acceptable outside of x86.

I wouldn't ack an arm patch which made it such that the files in a single
subdirectory fell in two "classes" like this, and I'd probably argue more
strongly against it if it was being used in common code.

As it is today only for x86 I suppose I have no real grounds to nack it,
except to preemptively nack any use of it outside of x86.

Ian.

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

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Jan Beulich]

>>> On 03.11.15 at 13:22, <ian.campbell@citrix.com> wrote:
> On Mon, 2015-11-02 at 09:11 -0700, Jan Beulich wrote:
>> > Does all of that fall out from a desire to reuse __FILE__? If so I'm
>> > inclined to suggest that -DBUILD_FILENAME_PREFIX="compat/" or whatever
>> > would seem likely to me to end up less strange (but maybe you tried
>> > that
>> > and it was worse?).
>> 
>> Yes to the first question. And no, I didn't try the alternative you
>> suggest, but discarded it as the uglier variant from the beginning.
>> In particular I dislike (parts of) file names to be specified on the
>> command line, rather than getting derived from what we have
>> anyway.
> 
> Hrm, ok.
> 
> What I was actually thinking (but not expressing very well in the example I
> gave) was that the code being compiled a second time in "compat" mode would
> be tagged as that, e.g. by listing them in obj-compat-y and adding
> something to CFLAGS (which may or may not look like a subpath, it might
> e.g. just be "(compat)").

I definitely want this to be a real path, i.e. a representation allowing
to connect thing to how the source file layout is without any guessing.

> Many of the double compiled files end up #defineing COMPAT, even without
> moving that to CFLAGS isn't that enough to key such a distinction on.

Of course that would be possible, but since that would mean
e.g. adding #ifdef-s it would clutter the code.

>> Considering that Andrew was fine with the x86 parts, I'd want to
>> change the approach (the x86 side of which I understand is of
>> particular concern to you) only if you're convinced this alternative
>> approach is sufficiently much better.
> 
> I'm mostly concerned with precedent being set by x86 and also implied by
> the common code Makefile infra which supports it causing people to think it
> is acceptable outside of x86.
> 
> I wouldn't ack an arm patch which made it such that the files in a single
> subdirectory fell in two "classes" like this, and I'd probably argue more
> strongly against it if it was being used in common code.

Okay, so for common code you didn't object to (but also didn't ack)
the change to compile the whole compat/ subtree from one level up.
That's certainly an option on x86 too, the more that the x86_64/
subtree is a remnant of x86_32 days only anyway. Just that doing
this will mean quite a bit more work (not the least because, to be
done properly, I think it implies merging files from x86_64/ into their
[formerly] shared files where sensible).

Jan

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Ian Campbell]

On Tue, 2015-11-03 at 05:50 -0700, Jan Beulich wrote:
> > > > 
> > > Considering that Andrew was fine with the x86 parts, I'd want to
> > > change the approach (the x86 side of which I understand is of
> > > particular concern to you) only if you're convinced this alternative
> > > approach is sufficiently much better.
> > 
> > I'm mostly concerned with precedent being set by x86 and also implied
> > by
> > the common code Makefile infra which supports it causing people to
> > think it
> > is acceptable outside of x86.
> > 
> > I wouldn't ack an arm patch which made it such that the files in a single
> > subdirectory fell in two "classes" like this, and I'd probably argue more
> > strongly against it if it was being used in common code.
> 
> Okay, so for common code you didn't object to (but also didn't ack)
> the change to compile the whole compat/ subtree from one level up.

Correct I find it tolerable enough if a given sub directory is either
recursed into or compiled from the parent directory, it's the mixing I
object to.

> That's certainly an option on x86 too, the more that the x86_64/
> subtree is a remnant of x86_32 days only anyway. Just that doing
> this will mean quite a bit more work (not the least because, to be
> done properly, I think it implies merging files from x86_64/ into their
> [formerly] shared files where sensible).

FWIW IMHO compiling arch/x86/x86_64/*.c from arch/x86/Makefile now and then
tackling the merging as and when it makes sense would be fine.

Ian.

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

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Jan Beulich]

>>> On 03.11.15 at 14:39, <ian.campbell@citrix.com> wrote:
> On Tue, 2015-11-03 at 05:50 -0700, Jan Beulich wrote:
>> That's certainly an option on x86 too, the more that the x86_64/
>> subtree is a remnant of x86_32 days only anyway. Just that doing
>> this will mean quite a bit more work (not the least because, to be
>> done properly, I think it implies merging files from x86_64/ into their
>> [formerly] shared files where sensible).
> 
> FWIW IMHO compiling arch/x86/x86_64/*.c from arch/x86/Makefile now and then
> tackling the merging as and when it makes sense would be fine.

Great. It's on my list to do this merge (in steps) anyway. So does
this mean I can go ahead and commit both this and patch 1, i.e.
at least kind-of-an-ack?

Jan

Re: [PATCH v2 2/5] compat: enforce distinguishable file names in symbol table

[Ian Campbell]

On Tue, 2015-11-03 at 08:31 -0700, Jan Beulich wrote:
> > > > On 03.11.15 at 14:39, <ian.campbell@citrix.com> wrote:
> > On Tue, 2015-11-03 at 05:50 -0700, Jan Beulich wrote:
> > > That's certainly an option on x86 too, the more that the x86_64/
> > > subtree is a remnant of x86_32 days only anyway. Just that doing
> > > this will mean quite a bit more work (not the least because, to be
> > > done properly, I think it implies merging files from x86_64/ into
> > > their
> > > [formerly] shared files where sensible).
> > 
> > FWIW IMHO compiling arch/x86/x86_64/*.c from arch/x86/Makefile now and
> > then
> > tackling the merging as and when it makes sense would be fine.
> 
> Great. It's on my list to do this merge (in steps) anyway. So does
> this mean I can go ahead and commit both this and patch 1, i.e.
> at least kind-of-an-ack?

Acked-by: Ian Campbell <ian.campbell@citrix.com>

with the provisos regarding common code and not setting a precedent for ARM
I mentioned before (which doesn't need reflecting in the ack in the commit
message, I'll just remember it)

Ian.

[PATCH v2 3/5] x86/mm: override stored file names for multiply built sources

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 1928 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>
---
v2: Introduce __OBJECT_FILE__.

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -42,10 +42,10 @@ ALL_OBJS-y               += $(BASEDIR)/x
 ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(x86)          += $(BASEDIR)/crypto/built_in.o
 
-CFLAGS += -fno-builtin -fno-common
+CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
 CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h
-CFLAGS += -nostdinc
+CFLAGS += '-D__OBJECT_FILE__="$@"'
 
 CFLAGS-$(XSM_ENABLE)    += -DXSM_ENABLE
 CFLAGS-$(FLASK_ENABLE)  += -DFLASK_ENABLE
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -21,6 +21,9 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"" __OBJECT_FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/paging.h>
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -18,6 +18,8 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"" __OBJECT_FILE__ "\"");
 
 #include <xen/domain_page.h>
 #include <xen/paging.h>
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -20,7 +20,9 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/config.h>
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"" __OBJECT_FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/trace.h>




[-- Attachment #2: x86-mm-rename-multiply-built.patch --]
[-- Type: text/plain, Size: 1987 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>
---
v2: Introduce __OBJECT_FILE__.

--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -42,10 +42,10 @@ ALL_OBJS-y               += $(BASEDIR)/x
 ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
 ALL_OBJS-$(x86)          += $(BASEDIR)/crypto/built_in.o
 
-CFLAGS += -fno-builtin -fno-common
+CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
 CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h
-CFLAGS += -nostdinc
+CFLAGS += '-D__OBJECT_FILE__="$@"'
 
 CFLAGS-$(XSM_ENABLE)    += -DXSM_ENABLE
 CFLAGS-$(FLASK_ENABLE)  += -DFLASK_ENABLE
--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -21,6 +21,9 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"" __OBJECT_FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/paging.h>
--- a/xen/arch/x86/mm/hap/guest_walk.c
+++ b/xen/arch/x86/mm/hap/guest_walk.c
@@ -18,6 +18,8 @@
  * this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"" __OBJECT_FILE__ "\"");
 
 #include <xen/domain_page.h>
 #include <xen/paging.h>
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -20,7 +20,9 @@
  * along with this program; If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <xen/config.h>
+/* Allow uniquely identifying static symbols in the 3 generated objects. */
+asm(".file \"" __OBJECT_FILE__ "\"");
+
 #include <xen/types.h>
 #include <xen/mm.h>
 #include <xen/trace.h>

[-- 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 v2 3/5] x86/mm: override stored file names for multiply built sources

[Andrew Cooper]

On 26/10/15 11:51, 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>

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

Re: [PATCH v2 3/5] x86/mm: override stored file names for multiply built sources

[Martin Pohlack]

On 26.10.2015 12:51, 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>
> ---
> v2: Introduce __OBJECT_FILE__.
> 
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -42,10 +42,10 @@ ALL_OBJS-y               += $(BASEDIR)/x
>  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
>  ALL_OBJS-$(x86)          += $(BASEDIR)/crypto/built_in.o
>  
> -CFLAGS += -fno-builtin -fno-common
> +CFLAGS += -nostdinc -fno-builtin -fno-common
>  CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
>  CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h
> -CFLAGS += -nostdinc
> +CFLAGS += '-D__OBJECT_FILE__="$@"'
>  
>  CFLAGS-$(XSM_ENABLE)    += -DXSM_ENABLE
>  CFLAGS-$(FLASK_ENABLE)  += -DFLASK_ENABLE
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -21,6 +21,9 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"" __OBJECT_FILE__ "\"");
> +

I wonder if using symlinks to the C files and compiling those would be a
more obvious way to set the file symbol to something unique.

Martin

Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH v2 3/5] x86/mm: override stored file names for multiply built sources

[George Dunlap]

On 26/10/15 14:34, Martin Pohlack wrote:
> On 26.10.2015 12:51, 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>
>> ---
>> v2: Introduce __OBJECT_FILE__.
>>
>> --- a/xen/Rules.mk
>> +++ b/xen/Rules.mk
>> @@ -42,10 +42,10 @@ ALL_OBJS-y               += $(BASEDIR)/x
>>  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
>>  ALL_OBJS-$(x86)          += $(BASEDIR)/crypto/built_in.o
>>  
>> -CFLAGS += -fno-builtin -fno-common
>> +CFLAGS += -nostdinc -fno-builtin -fno-common
>>  CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
>>  CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h
>> -CFLAGS += -nostdinc
>> +CFLAGS += '-D__OBJECT_FILE__="$@"'
>>  
>>  CFLAGS-$(XSM_ENABLE)    += -DXSM_ENABLE
>>  CFLAGS-$(FLASK_ENABLE)  += -DFLASK_ENABLE
>> --- a/xen/arch/x86/mm/guest_walk.c
>> +++ b/xen/arch/x86/mm/guest_walk.c
>> @@ -21,6 +21,9 @@
>>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>   */
>>  
>> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
>> +asm(".file \"" __OBJECT_FILE__ "\"");
>> +
> 
> I wonder if using symlinks to the C files and compiling those would be a
> more obvious way to set the file symbol to something unique.

That's an interesting idea.  My initial reactions:

1. Setting up the runes and such to make those links wouldn't be simpler
than the asm()-ery that Jan has here

2. Having random linked files lying around has the potential to cause
other forms of confusion (git needs to know to ignore them; developers
may not realize that editing one edits all the others, &c; even if they
do, they may accidentally open two files in an editor that doesn't
realize they're the same file, &c)

 -George

Re: [PATCH v2 3/5] x86/mm: override stored file names for multiply built sources

[Jan Beulich]

>>> On 26.10.15 at 15:57, <george.dunlap@citrix.com> wrote:
> On 26/10/15 14:34, Martin Pohlack wrote:
>> On 26.10.2015 12:51, 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>
>>> ---
>>> v2: Introduce __OBJECT_FILE__.
>>>
>>> --- a/xen/Rules.mk
>>> +++ b/xen/Rules.mk
>>> @@ -42,10 +42,10 @@ ALL_OBJS-y               += $(BASEDIR)/x
>>>  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
>>>  ALL_OBJS-$(x86)          += $(BASEDIR)/crypto/built_in.o
>>>  
>>> -CFLAGS += -fno-builtin -fno-common
>>> +CFLAGS += -nostdinc -fno-builtin -fno-common
>>>  CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
>>>  CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h
>>> -CFLAGS += -nostdinc
>>> +CFLAGS += '-D__OBJECT_FILE__="$@"'
>>>  
>>>  CFLAGS-$(XSM_ENABLE)    += -DXSM_ENABLE
>>>  CFLAGS-$(FLASK_ENABLE)  += -DFLASK_ENABLE
>>> --- a/xen/arch/x86/mm/guest_walk.c
>>> +++ b/xen/arch/x86/mm/guest_walk.c
>>> @@ -21,6 +21,9 @@
>>>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>>>   */
>>>  
>>> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
>>> +asm(".file \"" __OBJECT_FILE__ "\"");
>>> +
>> 
>> I wonder if using symlinks to the C files and compiling those would be a
>> more obvious way to set the file symbol to something unique.
> 
> That's an interesting idea.  My initial reactions:
> 
> 1. Setting up the runes and such to make those links wouldn't be simpler
> than the asm()-ery that Jan has here
> 
> 2. Having random linked files lying around has the potential to cause
> other forms of confusion (git needs to know to ignore them; developers
> may not realize that editing one edits all the others, &c; even if they
> do, they may accidentally open two files in an editor that doesn't
> realize they're the same file, &c)

Yeah, if we wanted to try to do without the asm()s here, I think
the next best approach would be to create real source files
#define-ing GUEST_PAGING_LEVELS and then include the main one.
But to be honest I think the asm() approach is still cheaper than
that or (as you say) the symlink one.

Jan

Re: [PATCH v2 3/5] x86/mm: override stored file names for multiply built sources

[George Dunlap]

On 26/10/15 11:51, 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: George Dunlap <george.dunlap@citrix.com>

> ---
> v2: Introduce __OBJECT_FILE__.
> 
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -42,10 +42,10 @@ ALL_OBJS-y               += $(BASEDIR)/x
>  ALL_OBJS-y               += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
>  ALL_OBJS-$(x86)          += $(BASEDIR)/crypto/built_in.o
>  
> -CFLAGS += -fno-builtin -fno-common
> +CFLAGS += -nostdinc -fno-builtin -fno-common
>  CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
>  CFLAGS += -pipe -g -D__XEN__ -include $(BASEDIR)/include/xen/config.h
> -CFLAGS += -nostdinc
> +CFLAGS += '-D__OBJECT_FILE__="$@"'
>  
>  CFLAGS-$(XSM_ENABLE)    += -DXSM_ENABLE
>  CFLAGS-$(FLASK_ENABLE)  += -DFLASK_ENABLE
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -21,6 +21,9 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"" __OBJECT_FILE__ "\"");
> +
>  #include <xen/types.h>
>  #include <xen/mm.h>
>  #include <xen/paging.h>
> --- a/xen/arch/x86/mm/hap/guest_walk.c
> +++ b/xen/arch/x86/mm/hap/guest_walk.c
> @@ -18,6 +18,8 @@
>   * this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"" __OBJECT_FILE__ "\"");
>  
>  #include <xen/domain_page.h>
>  #include <xen/paging.h>
> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -20,7 +20,9 @@
>   * along with this program; If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> -#include <xen/config.h>
> +/* Allow uniquely identifying static symbols in the 3 generated objects. */
> +asm(".file \"" __OBJECT_FILE__ "\"");
> +
>  #include <xen/types.h>
>  #include <xen/mm.h>
>  #include <xen/trace.h>
> 
> 
> 

[PATCH v2 4/5] x86/mm: build map_domain_gfn() just once

[Jan Beulich]

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

It doesn't depend on GUEST_PAGING_LEVELS. Moving the function to p2m.c
at once allows a bogus #define/#include pair to be removed from
hap/nested_ept.c.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: To ensure no dependency on GUEST_PAGING_LEVELS, move the function
    to p2m.c.

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -88,46 +88,6 @@ 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) */
-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)
-{
-    struct page_info *page;
-    void *map;
-
-    /* Translate the gfn, unsharing if shared */
-    page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL,
-                                 q);
-    if ( p2m_is_paging(*p2mt) )
-    {
-        ASSERT(p2m_is_hostp2m(p2m));
-        if ( page )
-            put_page(page);
-        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
-        *rc = _PAGE_PAGED;
-        return NULL;
-    }
-    if ( p2m_is_shared(*p2mt) )
-    {
-        if ( page )
-            put_page(page);
-        *rc = _PAGE_SHARED;
-        return NULL;
-    }
-    if ( !page )
-    {
-        *rc |= _PAGE_PRESENT;
-        return NULL;
-    }
-    *mfn = _mfn(page_to_mfn(page));
-    ASSERT(mfn_valid(mfn_x(*mfn)));
-
-    map = map_domain_page(*mfn);
-    return map;
-}
-
-
 /* Walk the guest pagetables, after the manner of a hardware walker. */
 /* Because the walk is essentially random, it can cause a deadlock 
  * warning in the p2m locking code. Highly unlikely this is an actual
--- a/xen/arch/x86/mm/hap/nested_ept.c
+++ b/xen/arch/x86/mm/hap/nested_ept.c
@@ -34,10 +34,6 @@
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/vmx/vvmx.h>
 
-/* EPT always use 4-level paging structure */
-#define GUEST_PAGING_LEVELS 4
-#include <asm/guest_pt.h>
-
 /* Must reserved bits in all level entries  */
 #define EPT_MUST_RSV_BITS (((1ull << PADDR_BITS) - 1) & \
                            ~((1ull << paddr_bits) - 1))
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2053,6 +2053,44 @@ unsigned long paging_gva_to_gfn(struct v
     return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
 }
 
+/*
+ * 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)
+{
+    struct page_info *page;
+
+    /* Translate the gfn, unsharing if shared. */
+    page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL, q);
+    if ( p2m_is_paging(*p2mt) )
+    {
+        ASSERT(p2m_is_hostp2m(p2m));
+        if ( page )
+            put_page(page);
+        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
+        *rc = _PAGE_PAGED;
+        return NULL;
+    }
+    if ( p2m_is_shared(*p2mt) )
+    {
+        if ( page )
+            put_page(page);
+        *rc = _PAGE_SHARED;
+        return NULL;
+    }
+    if ( !page )
+    {
+        *rc |= _PAGE_PRESENT;
+        return NULL;
+    }
+    *mfn = page_to_mfn(page);
+    ASSERT(mfn_valid(*mfn));
+
+    return map_domain_page(*mfn);
+}
+
 int map_mmio_regions(struct domain *d,
                      unsigned long start_gfn,
                      unsigned long nr,
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -305,10 +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);
 
 extern uint32_t 
 guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, unsigned long va,
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -680,6 +680,9 @@ int p2m_set_entry(struct p2m_domain *p2m
 /* Set up function pointers for PT implementation: only for use by p2m code */
 extern void p2m_pt_init(struct p2m_domain *p2m);
 
+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);
+
 /* Debugging and auditing of the P2M code? */
 #ifndef NDEBUG
 #define P2M_AUDIT     1



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

x86/mm: build map_domain_gfn() just once

It doesn't depend on GUEST_PAGING_LEVELS. Moving the function to p2m.c
at once allows a bogus #define/#include pair to be removed from
hap/nested_ept.c.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: To ensure no dependency on GUEST_PAGING_LEVELS, move the function
    to p2m.c.

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -88,46 +88,6 @@ 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) */
-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)
-{
-    struct page_info *page;
-    void *map;
-
-    /* Translate the gfn, unsharing if shared */
-    page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL,
-                                 q);
-    if ( p2m_is_paging(*p2mt) )
-    {
-        ASSERT(p2m_is_hostp2m(p2m));
-        if ( page )
-            put_page(page);
-        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
-        *rc = _PAGE_PAGED;
-        return NULL;
-    }
-    if ( p2m_is_shared(*p2mt) )
-    {
-        if ( page )
-            put_page(page);
-        *rc = _PAGE_SHARED;
-        return NULL;
-    }
-    if ( !page )
-    {
-        *rc |= _PAGE_PRESENT;
-        return NULL;
-    }
-    *mfn = _mfn(page_to_mfn(page));
-    ASSERT(mfn_valid(mfn_x(*mfn)));
-
-    map = map_domain_page(*mfn);
-    return map;
-}
-
-
 /* Walk the guest pagetables, after the manner of a hardware walker. */
 /* Because the walk is essentially random, it can cause a deadlock 
  * warning in the p2m locking code. Highly unlikely this is an actual
--- a/xen/arch/x86/mm/hap/nested_ept.c
+++ b/xen/arch/x86/mm/hap/nested_ept.c
@@ -34,10 +34,6 @@
 #include <asm/hvm/vmx/vmx.h>
 #include <asm/hvm/vmx/vvmx.h>
 
-/* EPT always use 4-level paging structure */
-#define GUEST_PAGING_LEVELS 4
-#include <asm/guest_pt.h>
-
 /* Must reserved bits in all level entries  */
 #define EPT_MUST_RSV_BITS (((1ull << PADDR_BITS) - 1) & \
                            ~((1ull << paddr_bits) - 1))
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2053,6 +2053,44 @@ unsigned long paging_gva_to_gfn(struct v
     return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
 }
 
+/*
+ * 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)
+{
+    struct page_info *page;
+
+    /* Translate the gfn, unsharing if shared. */
+    page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL, q);
+    if ( p2m_is_paging(*p2mt) )
+    {
+        ASSERT(p2m_is_hostp2m(p2m));
+        if ( page )
+            put_page(page);
+        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
+        *rc = _PAGE_PAGED;
+        return NULL;
+    }
+    if ( p2m_is_shared(*p2mt) )
+    {
+        if ( page )
+            put_page(page);
+        *rc = _PAGE_SHARED;
+        return NULL;
+    }
+    if ( !page )
+    {
+        *rc |= _PAGE_PRESENT;
+        return NULL;
+    }
+    *mfn = page_to_mfn(page);
+    ASSERT(mfn_valid(*mfn));
+
+    return map_domain_page(*mfn);
+}
+
 int map_mmio_regions(struct domain *d,
                      unsigned long start_gfn,
                      unsigned long nr,
--- a/xen/include/asm-x86/guest_pt.h
+++ b/xen/include/asm-x86/guest_pt.h
@@ -305,10 +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);
 
 extern uint32_t 
 guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, unsigned long va,
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -680,6 +680,9 @@ int p2m_set_entry(struct p2m_domain *p2m
 /* Set up function pointers for PT implementation: only for use by p2m code */
 extern void p2m_pt_init(struct p2m_domain *p2m);
 
+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);
+
 /* Debugging and auditing of the P2M code? */
 #ifndef NDEBUG
 #define P2M_AUDIT     1

[-- 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 v2 4/5] x86/mm: build map_domain_gfn() just once

[Andrew Cooper]

On 26/10/15 11:52, Jan Beulich wrote:
> It doesn't depend on GUEST_PAGING_LEVELS. Moving the function to p2m.c
> at once allows a bogus #define/#include pair to be removed from
> hap/nested_ept.c.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

Re: [PATCH v2 4/5] x86/mm: build map_domain_gfn() just once

[George Dunlap]

On 26/10/15 11:52, Jan Beulich wrote:
> It doesn't depend on GUEST_PAGING_LEVELS. Moving the function to p2m.c
> at once allows a bogus #define/#include pair to be removed from
> hap/nested_ept.c.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: To ensure no dependency on GUEST_PAGING_LEVELS, move the function
>     to p2m.c.

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

> 
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -88,46 +88,6 @@ 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) */
> -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)
> -{
> -    struct page_info *page;
> -    void *map;
> -
> -    /* Translate the gfn, unsharing if shared */
> -    page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL,
> -                                 q);
> -    if ( p2m_is_paging(*p2mt) )
> -    {
> -        ASSERT(p2m_is_hostp2m(p2m));
> -        if ( page )
> -            put_page(page);
> -        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
> -        *rc = _PAGE_PAGED;
> -        return NULL;
> -    }
> -    if ( p2m_is_shared(*p2mt) )
> -    {
> -        if ( page )
> -            put_page(page);
> -        *rc = _PAGE_SHARED;
> -        return NULL;
> -    }
> -    if ( !page )
> -    {
> -        *rc |= _PAGE_PRESENT;
> -        return NULL;
> -    }
> -    *mfn = _mfn(page_to_mfn(page));
> -    ASSERT(mfn_valid(mfn_x(*mfn)));
> -
> -    map = map_domain_page(*mfn);
> -    return map;
> -}
> -
> -
>  /* Walk the guest pagetables, after the manner of a hardware walker. */
>  /* Because the walk is essentially random, it can cause a deadlock 
>   * warning in the p2m locking code. Highly unlikely this is an actual
> --- a/xen/arch/x86/mm/hap/nested_ept.c
> +++ b/xen/arch/x86/mm/hap/nested_ept.c
> @@ -34,10 +34,6 @@
>  #include <asm/hvm/vmx/vmx.h>
>  #include <asm/hvm/vmx/vvmx.h>
>  
> -/* EPT always use 4-level paging structure */
> -#define GUEST_PAGING_LEVELS 4
> -#include <asm/guest_pt.h>
> -
>  /* Must reserved bits in all level entries  */
>  #define EPT_MUST_RSV_BITS (((1ull << PADDR_BITS) - 1) & \
>                             ~((1ull << paddr_bits) - 1))
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2053,6 +2053,44 @@ unsigned long paging_gva_to_gfn(struct v
>      return hostmode->gva_to_gfn(v, hostp2m, va, pfec);
>  }
>  
> +/*
> + * 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)
> +{
> +    struct page_info *page;
> +
> +    /* Translate the gfn, unsharing if shared. */
> +    page = get_page_from_gfn_p2m(p2m->domain, p2m, gfn_x(gfn), p2mt, NULL, q);
> +    if ( p2m_is_paging(*p2mt) )
> +    {
> +        ASSERT(p2m_is_hostp2m(p2m));
> +        if ( page )
> +            put_page(page);
> +        p2m_mem_paging_populate(p2m->domain, gfn_x(gfn));
> +        *rc = _PAGE_PAGED;
> +        return NULL;
> +    }
> +    if ( p2m_is_shared(*p2mt) )
> +    {
> +        if ( page )
> +            put_page(page);
> +        *rc = _PAGE_SHARED;
> +        return NULL;
> +    }
> +    if ( !page )
> +    {
> +        *rc |= _PAGE_PRESENT;
> +        return NULL;
> +    }
> +    *mfn = page_to_mfn(page);
> +    ASSERT(mfn_valid(*mfn));
> +
> +    return map_domain_page(*mfn);
> +}
> +
>  int map_mmio_regions(struct domain *d,
>                       unsigned long start_gfn,
>                       unsigned long nr,
> --- a/xen/include/asm-x86/guest_pt.h
> +++ b/xen/include/asm-x86/guest_pt.h
> @@ -305,10 +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);
>  
>  extern uint32_t 
>  guest_walk_tables(struct vcpu *v, struct p2m_domain *p2m, unsigned long va,
> --- a/xen/include/asm-x86/p2m.h
> +++ b/xen/include/asm-x86/p2m.h
> @@ -680,6 +680,9 @@ int p2m_set_entry(struct p2m_domain *p2m
>  /* Set up function pointers for PT implementation: only for use by p2m code */
>  extern void p2m_pt_init(struct p2m_domain *p2m);
>  
> +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);
> +
>  /* Debugging and auditing of the P2M code? */
>  #ifndef NDEBUG
>  #define P2M_AUDIT     1
> 
> 

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

[Jan Beulich]

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

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>
---
v2: Re-base on top of earlier changes.

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -32,30 +32,32 @@ asm(".file \"" __OBJECT_FILE__ "\"");
 #include <asm/page.h>
 #include <asm/guest_pt.h>
 
+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;
@@ -65,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: 3255 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>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v2: Re-base on top of earlier changes.

--- a/xen/arch/x86/mm/guest_walk.c
+++ b/xen/arch/x86/mm/guest_walk.c
@@ -32,30 +32,32 @@ asm(".file \"" __OBJECT_FILE__ "\"");
 #include <asm/page.h>
 #include <asm/guest_pt.h>
 
+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;
@@ -65,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 v2 5/5] x86/mm: only a single instance of gw_page_flags[] is needed

[George Dunlap]

On 26/10/15 11:53, 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>
> ---
> v2: Re-base on top of earlier changes.

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

> 
> --- a/xen/arch/x86/mm/guest_walk.c
> +++ b/xen/arch/x86/mm/guest_walk.c
> @@ -32,30 +32,32 @@ asm(".file \"" __OBJECT_FILE__ "\"");
>  #include <asm/page.h>
>  #include <asm/guest_pt.h>
>  
> +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;
> @@ -65,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.
> 
> 
>