[PATCH v5 0/5] xen: public interface and foreign struct check changes for arm

[PATCH v5 0/5] xen: public interface and foreign struct check changes for arm

[Ian Campbell]

I last posted this back in April to critical acclaim (AKA near total
silence).

I'm not sure who looks after tools/include/xen-foreign. I had thought it
was Jan but I think I was confused and was thinking of the semi-related
xen/include/compat stuff. IOW I think nobody felt "responsible".

Unless there's any objection lets just treat this as coming under tools.
The alternative is that since it is related to hypervisor ABI it comes
under general hypervisor stuff.

Since v4:

Rebased onto current staging tree, no other changes

[PATCH 1/5] xen/compat: support XEN_HAVE_FOO ifdefs in public interface

[Ian Campbell]

This allows us expose or hide interface features on different architectures
without requiring nasty arch-specific ifdeffery.

Preserves any #ifdef with a XEN_HAVE_* symbol name, as well as any #else or

The ifdef symbol becomes COMPAT_HAVE in the compat versions so that
architectures can enable or disable interfaces for compat mode too. (This
actually just fell out of the way the existing stuff works and it didn't seem
worth jumping through hoops to make the name remain XEN_HAVE).

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
 xen/tools/compat-build-header.py |    3 +++
 xen/tools/compat-build-source.py |    3 +++
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/xen/tools/compat-build-header.py b/xen/tools/compat-build-header.py
index fba2f37..e296062 100755
--- a/xen/tools/compat-build-header.py
+++ b/xen/tools/compat-build-header.py
@@ -4,6 +4,9 @@ import re,sys
 
 pats = [
  [ r"__InClUdE__(.*)", r"#include\1\n#pragma pack(4)" ],
+ [ r"__IfDeF__ (XEN_HAVE.*)", r"#ifdef \1" ],
+ [ r"__ElSe__", r"#else" ],
+ [ r"__EnDif__", r"#endif" ],
  [ r"\"xen-compat.h\"", r"<public/xen-compat.h>" ],
  [ r"(struct|union|enum)\s+(xen_?)?(\w)", r"\1 compat_\3" ],
  [ r"@KeeP@", r"" ],
diff --git a/xen/tools/compat-build-source.py b/xen/tools/compat-build-source.py
index 3906b71..55206e6 100755
--- a/xen/tools/compat-build-source.py
+++ b/xen/tools/compat-build-source.py
@@ -4,6 +4,9 @@ import re,sys
 
 pats = [
  [ r"^\s*#\s*include\s+", r"__InClUdE__ " ],
+ [ r"^\s*#\s*ifdef (XEN_HAVE.*)\s+", r"__IfDeF__ \1" ],
+ [ r"^\s*#\s*else /\* (XEN_HAVE.*) \*/\s+", r"__ElSe__" ],
+ [ r"^\s*#\s*endif /\* (XEN_HAVE.*) \*/\s+", r"__EnDif__" ],
  [ r"^\s*#\s*define\s+([A-Z_]*_GUEST_HANDLE)", r"#define HIDE_\1" ],
  [ r"^\s*#\s*define\s+([a-z_]*_guest_handle)", r"#define hide_\1" ],
  [ r"XEN_GUEST_HANDLE(_[0-9A-Fa-f]+)?", r"COMPAT_HANDLE" ],
-- 
1.7.2.5

[PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Ian Campbell]

Most of this struct is PV MMU specific and it is not used on ARM at all.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Jan Beulich <JBeulich@suse.com>
Cc: Keir (Xen.org) <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v3: Renamed from "xen: make start_info x86 specific." and use an ifdef instead
    of moving the definition.
---
 tools/libxc/xenctrl.h             |    5 ++---
 xen/include/public/arch-x86/xen.h |    3 +++
 xen/include/public/xen.h          |    3 ++-
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/tools/libxc/xenctrl.h b/tools/libxc/xenctrl.h
index 388a9c3..f2cebaf 100644
--- a/tools/libxc/xenctrl.h
+++ b/tools/libxc/xenctrl.h
@@ -408,15 +408,14 @@ typedef union
     shared_info_t s;
 } shared_info_any_t;
 
+#if defined(__i386__) || defined(__x86_64__)
 typedef union
 {
-#if defined(__i386__) || defined(__x86_64__)
     start_info_x86_64_t x64;
     start_info_x86_32_t x32;
-#endif
     start_info_t s;
 } start_info_any_t;
-
+#endif
 
 int xc_domain_create(xc_interface *xch,
                      uint32_t ssidref,
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index b7f6a51..c528e91 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
 #define PRI_xen_pfn "lx"
 #endif
 
+#define XEN_HAVE_PV_GUEST_ENTRY 1
+#define COMPAT_HAVE_PV_GUEST_ENTRY 1
+
 /*
  * `incontents 200 segdesc Segment Descriptor Tables
  */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 3cab74f..2414e7e 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -716,7 +716,7 @@ typedef struct shared_info shared_info_t;
  * 32-bit and runs under a 64-bit hypervisor should _NOT_ use two of the
  * pages preceding pt_base and mark them as reserved/unused.
  */
-
+#ifdef XEN_HAVE_PV_GUEST_ENTRY
 #define MAX_GUEST_CMDLINE 1024
 struct start_info {
     /* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.    */
@@ -756,6 +756,7 @@ typedef struct start_info start_info_t;
 #define console_mfn    console.domU.mfn
 #define console_evtchn console.domU.evtchn
 #endif
+#endif /* XEN_HAVE_PV_GUEST_ENTRY */
 
 /* These flags are passed in the 'flags' field of start_info_t. */
 #define SIF_PRIVILEGED    (1<<0)  /* Is the domain privileged? */
-- 
1.7.2.5

[PATCH 3/5] xen: arm: include public/xen.h in foreign interface checking

[Ian Campbell]

mkheader.py doesn't cope with
	struct foo { };
so add a newline.

Define unsigned long and long to a non-existent type on ARM so as to catch
their use.

Teach mkheader.py to cope with structs which are ifdef'd.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir (Xen.org) <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
v3: rework for ifdeffed instead of moved start_info.
---
 tools/include/xen-foreign/Makefile       |    4 ++--
 tools/include/xen-foreign/mkheader.py    |   14 +++++++++-----
 tools/include/xen-foreign/reference.size |   10 +++++-----
 tools/include/xen-foreign/structs.py     |    2 ++
 xen/include/public/arch-arm.h            |    8 +++++---
 xen/include/public/xen.h                 |    2 +-
 6 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/tools/include/xen-foreign/Makefile b/tools/include/xen-foreign/Makefile
index 8e0be83..06b844c 100644
--- a/tools/include/xen-foreign/Makefile
+++ b/tools/include/xen-foreign/Makefile
@@ -22,10 +22,10 @@ check-headers: checker
 	diff -u reference.size tmp.size
 	rm tmp.size
 
-arm32.h: mkheader.py structs.py $(ROOT)/arch-arm.h
+arm32.h: mkheader.py structs.py $(ROOT)/arch-arm.h $(ROOT)/xen.h
 	$(PYTHON) $< $* $@ $(filter %.h,$^)
 
-arm64.h: mkheader.py structs.py $(ROOT)/arch-arm.h
+arm64.h: mkheader.py structs.py $(ROOT)/arch-arm.h $(ROOT)/xen.h
 	$(PYTHON) $< $* $@ $(filter %.h,$^)
 
 x86_32.h: mkheader.py structs.py $(ROOT)/arch-x86/xen-x86_32.h $(ROOT)/arch-x86/xen.h $(ROOT)/xen.h
diff --git a/tools/include/xen-foreign/mkheader.py b/tools/include/xen-foreign/mkheader.py
index b19292f..0504cb8 100644
--- a/tools/include/xen-foreign/mkheader.py
+++ b/tools/include/xen-foreign/mkheader.py
@@ -18,8 +18,8 @@ footer = {};
 
 #arm
 inttypes["arm32"] = {
-    "unsigned long" : "uint32_t",
-    "long"          : "uint32_t",
+    "unsigned long" : "__danger_unsigned_long_on_arm32",
+    "long"          : "__danger_long_on_arm32",
     "xen_pfn_t"     : "__align8__ uint64_t",
     "xen_ulong_t"   : "__align8__ uint64_t",
     "uint64_t"      : "__align8__ uint64_t",
@@ -124,6 +124,8 @@ if arch in header:
     output += header[arch];
     output += "\n";
 
+defined = {}
+
 # add defines to output
 for line in re.findall("#define[^\n]+", input):
     for define in defines:
@@ -131,6 +133,7 @@ for line in re.findall("#define[^\n]+", input):
         match = re.search(regex, line);
         if None == match:
             continue;
+        defined[define] = 1
         if define.upper()[0] == define[0]:
             replace = define + "_" + arch.upper();
         else:
@@ -156,12 +159,13 @@ for union in unions:
 
 # add structs to output
 for struct in structs:
-    regex = "struct\s+%s\s*\{(.*?)\n\};" % struct;
+    regex = "(?:#ifdef ([A-Z_]+))?\nstruct\s+%s\s*\{(.*?)\n\};" % struct;
     match = re.search(regex, input, re.S)
-    if None == match:
+    if None == match or \
+           (match.group(1) is not None and match.group(1) not in defined):
         output += "#define %s_has_no_%s 1\n" % (arch, struct);
     else:
-        output += "struct %s_%s {%s\n};\n" % (struct, arch, match.group(1));
+        output += "struct %s_%s {%s\n};\n" % (struct, arch, match.group(2));
         output += "typedef struct %s_%s %s_%s_t;\n" % (struct, arch, struct, arch);
     output += "\n";
 
diff --git a/tools/include/xen-foreign/reference.size b/tools/include/xen-foreign/reference.size
index de36455..b3347b4 100644
--- a/tools/include/xen-foreign/reference.size
+++ b/tools/include/xen-foreign/reference.size
@@ -6,9 +6,9 @@ trap_info                 |       -       -       8      16
 cpu_user_regs             |       -       -      68     200
 vcpu_guest_core_regs      |     304     304       -       -
 vcpu_guest_context        |     336     336    2800    5168
-arch_vcpu_info            |       -       -      24      16
-vcpu_time_info            |       -       -      32      32
-vcpu_info                 |       -       -      64      64
-arch_shared_info          |       -       -     268     280
-shared_info               |       -       -    2584    3368
+arch_vcpu_info            |       0       0      24      16
+vcpu_time_info            |      32      32      32      32
+vcpu_info                 |      48      48      64      64
+arch_shared_info          |       0       0     268     280
+shared_info               |    1088    1088    2584    3368
 
diff --git a/tools/include/xen-foreign/structs.py b/tools/include/xen-foreign/structs.py
index 0b33a77..476eb85 100644
--- a/tools/include/xen-foreign/structs.py
+++ b/tools/include/xen-foreign/structs.py
@@ -19,6 +19,8 @@ defines = [ "__arm__",
             "__i386__",
             "__x86_64__",
 
+            "XEN_HAVE_PV_GUEST_ENTRY",
+
             # arm
             # None
 
diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h
index 8aa62d3..93c420c 100644
--- a/xen/include/public/arch-arm.h
+++ b/xen/include/public/arch-arm.h
@@ -195,14 +195,16 @@ struct vcpu_guest_context {
 typedef struct vcpu_guest_context vcpu_guest_context_t;
 DEFINE_XEN_GUEST_HANDLE(vcpu_guest_context_t);
 
-struct arch_vcpu_info { };
+struct arch_vcpu_info {
+};
 typedef struct arch_vcpu_info arch_vcpu_info_t;
 
-struct arch_shared_info { };
+struct arch_shared_info {
+};
 typedef struct arch_shared_info arch_shared_info_t;
 typedef uint64_t xen_callback_t;
 
-#endif /* ifndef __ASSEMBLY __ */
+#endif
 
 /* PSR bits (CPSR, SPSR)*/
 
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2414e7e..037540d 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -717,7 +717,6 @@ typedef struct shared_info shared_info_t;
  * pages preceding pt_base and mark them as reserved/unused.
  */
 #ifdef XEN_HAVE_PV_GUEST_ENTRY
-#define MAX_GUEST_CMDLINE 1024
 struct start_info {
     /* THE FOLLOWING ARE FILLED IN BOTH ON INITIAL BOOT AND ON RESUME.    */
     char magic[32];             /* "xen-<version>-<platform>".            */
@@ -744,6 +743,7 @@ struct start_info {
                                 /* (PFN of pre-loaded module if           */
                                 /*  SIF_MOD_START_PFN set in flags).      */
     unsigned long mod_len;      /* Size (bytes) of pre-loaded module.     */
+#define MAX_GUEST_CMDLINE 1024
     int8_t cmd_line[MAX_GUEST_CMDLINE];
     /* The pfn range here covers both page table and p->m table frames.   */
     unsigned long first_p2m_pfn;/* 1st pfn forming initial P->M table.    */
-- 
1.7.2.5

[PATCH 4/5] tools: foreign: add checks for compatible architectures

[Ian Campbell]

That is architectures whose struct layout must be identical. Use this for arm32
and arm64.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Jan Beulich <JBeulich@suse.com>
Cc: Keir (Xen.org) <keir@xen.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 tools/include/xen-foreign/mkchecker.py |   21 +++++++++++++++++++--
 tools/include/xen-foreign/structs.py   |    5 +++++
 2 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/tools/include/xen-foreign/mkchecker.py b/tools/include/xen-foreign/mkchecker.py
index 66c17b1..fdad869 100644
--- a/tools/include/xen-foreign/mkchecker.py
+++ b/tools/include/xen-foreign/mkchecker.py
@@ -1,7 +1,7 @@
 #!/usr/bin/python
 
 import sys;
-from structs import structs;
+from structs import structs, compat_arches;
 
 # command line arguments
 outfile = sys.argv[1];
@@ -37,10 +37,27 @@ for struct in structs:
     f.write('\tprintf("%%-25s |", "%s");\n' % struct);
     for a in archs:
         s = struct + "_" + a;
+        if compat_arches.has_key(a):
+            compat = compat_arches[a]
+            c = struct + "_" + compat;
+        else:
+            compat = None
         f.write('#ifdef %s_has_no_%s\n' % (a, struct));
-        f.write('\tprintf("%8s", "-");\n');
+        f.write('\tprintf("%8s",\n');
+        if compat:
+            f.write('# ifndef %s_has_no_%s\n' % (compat, struct));
+            f.write('\t\t"!"\n');
+            f.write('# else\n')
+            f.write('\t\t"-"\n');
+            f.write('# endif\n')
+        else:
+            f.write('\t\t"-"\n');
+        f.write('\t);\n')
         f.write("#else\n");
         f.write('\tprintf("%%8zd", sizeof(struct %s));\n' % s);
+        if compat:
+            f.write('\tif (sizeof(struct %s) != sizeof(struct %s))\n' % (s, c))
+            f.write('\t\tprintf("!");\n')
         f.write("#endif\n");
 
     f.write('\tprintf("\\n");\n\n');
diff --git a/tools/include/xen-foreign/structs.py b/tools/include/xen-foreign/structs.py
index 476eb85..3d9f2fe 100644
--- a/tools/include/xen-foreign/structs.py
+++ b/tools/include/xen-foreign/structs.py
@@ -58,3 +58,8 @@ defines = [ "__arm__",
             "XEN_LEGACY_MAX_VCPUS",
             "MAX_GUEST_CMDLINE" ];
 
+# Architectures which must be compatible, i.e. identical
+compat_arches = {
+    'arm32': 'arm64',
+    'arm64': 'arm32',
+}
-- 
1.7.2.5

[PATCH 5/5] xen: remove evtchn_upcall_mask from interface on ARM

[Ian Campbell]

On ARM event-channel upcalls are masked using the hardware's interrupt mask
bit and not by a software bit.

Leaving this field present in the interface has caused some confusion already
and is liable to mean it gets inadvertently used in the future. So arrange for
this field to be turned into a padding field on ARM by introducing a
XEN_HAVE_PV_UPCALL_MASK define.

This bit is also unused for x86 PV-on-HVM guests, but we can't realistically
distinguish those from x86 PV guests in the headers.

Add a per-arch vcpu_event_delivery_is_enabled function to replace the single
open coded use of evtchn_upcall_mask in common code (in a debug keyhandler).
The existing local_event_delivery_is_enabled, which operates only on current,
was unimplemented on ARM and unused on x86, so remove it.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: keir@xen.org
Cc: jbeulich@suse.com
---
This patch build on top of Stefano's "xen/arm: trap guest WFI" patch.
---
 xen/common/keyhandler.c           |    2 +-
 xen/include/asm-arm/event.h       |   13 +++++++------
 xen/include/asm-x86/event.h       |   10 +++++-----
 xen/include/public/arch-x86/xen.h |    3 +++
 xen/include/public/xen.h          |    4 ++++
 5 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c
index 5072133..5725f90 100644
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -293,7 +293,7 @@ static void dump_domains(unsigned char key)
                    v->vcpu_id, v->processor,
                    v->is_running ? 'T':'F', v->poll_evtchn,
                    vcpu_info(v, evtchn_upcall_pending),
-                   vcpu_info(v, evtchn_upcall_mask));
+                   !vcpu_event_delivery_is_enabled(v));
             cpuset_print(tmpstr, sizeof(tmpstr), v->vcpu_dirty_cpumask);
             printk("dirty_cpus=%s ", tmpstr);
             cpuset_print(tmpstr, sizeof(tmpstr), v->cpu_affinity);
diff --git a/xen/include/asm-arm/event.h b/xen/include/asm-arm/event.h
index ed05901..04d854f 100644
--- a/xen/include/asm-arm/event.h
+++ b/xen/include/asm-arm/event.h
@@ -7,6 +7,12 @@
 void vcpu_kick(struct vcpu *v);
 void vcpu_mark_events_pending(struct vcpu *v);
 
+static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
+{
+    struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs;
+    return !(regs->cpsr & PSR_IRQ_MASK);
+}
+
 static inline int local_events_need_delivery_nomask(void)
 {
     struct pending_irq *p = irq_to_pending(current, VGIC_IRQ_EVTCHN_CALLBACK);
@@ -31,16 +37,11 @@ static inline int local_events_need_delivery_nomask(void)
 
 static inline int local_events_need_delivery(void)
 {
-    struct cpu_user_regs *regs = guest_cpu_user_regs();
-
-    /* guest IRQs are masked */
-    if ( (regs->cpsr & PSR_IRQ_MASK) )
+    if ( !vcpu_event_delivery_is_enabled(current) )
         return 0;
     return local_events_need_delivery_nomask();
 }
 
-int local_event_delivery_is_enabled(void);
-
 static inline void local_event_delivery_enable(void)
 {
     struct cpu_user_regs *regs = guest_cpu_user_regs();
diff --git a/xen/include/asm-x86/event.h b/xen/include/asm-x86/event.h
index 06057c7..7edeb5b 100644
--- a/xen/include/asm-x86/event.h
+++ b/xen/include/asm-x86/event.h
@@ -14,6 +14,11 @@
 void vcpu_kick(struct vcpu *v);
 void vcpu_mark_events_pending(struct vcpu *v);
 
+static inline int vcpu_event_delivery_is_enabled(struct vcpu *v)
+{
+    return !vcpu_info(v, evtchn_upcall_mask);
+}
+
 int hvm_local_events_need_delivery(struct vcpu *v);
 static inline int local_events_need_delivery(void)
 {
@@ -23,11 +28,6 @@ static inline int local_events_need_delivery(void)
              !vcpu_info(v, evtchn_upcall_mask)));
 }
 
-static inline int local_event_delivery_is_enabled(void)
-{
-    return !vcpu_info(current, evtchn_upcall_mask);
-}
-
 static inline void local_event_delivery_disable(void)
 {
     vcpu_info(current, evtchn_upcall_mask) = 1;
diff --git a/xen/include/public/arch-x86/xen.h b/xen/include/public/arch-x86/xen.h
index c528e91..5c4b08e 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -73,6 +73,9 @@ typedef unsigned long xen_pfn_t;
 #define XEN_HAVE_PV_GUEST_ENTRY 1
 #define COMPAT_HAVE_PV_GUEST_ENTRY 1
 
+#define XEN_HAVE_PV_UPCALL_MASK 1
+#define COMPAT_HAVE_PV_UPCALL_MASK 1
+
 /*
  * `incontents 200 segdesc Segment Descriptor Tables
  */
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 037540d..2a40970 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -612,7 +612,11 @@ struct vcpu_info {
      * to block: this avoids wakeup-waiting races.
      */
     uint8_t evtchn_upcall_pending;
+#ifdef XEN_HAVE_PV_UPCALL_MASK
     uint8_t evtchn_upcall_mask;
+#else /* XEN_HAVE_PV_UPCALL_MASK */
+    uint8_t pad0;
+#endif /* XEN_HAVE_PV_UPCALL_MASK */
     xen_ulong_t evtchn_pending_sel;
     struct arch_vcpu_info arch;
     struct vcpu_time_info time;
-- 
1.7.2.5

Re: [PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Jan Beulich]

>>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
>  #define PRI_xen_pfn "lx"
>  #endif
>  
> +#define XEN_HAVE_PV_GUEST_ENTRY 1
> +#define COMPAT_HAVE_PV_GUEST_ENTRY 1

This ought to nevertheless have a XEN_ prefix, if it really is
needed at all (didn't spot yet where it's being consumed).

Jan

Re: [PATCH 5/5] xen: remove evtchn_upcall_mask from interface on ARM

[Jan Beulich]

>>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -73,6 +73,9 @@ typedef unsigned long xen_pfn_t;
>  #define XEN_HAVE_PV_GUEST_ENTRY 1
>  #define COMPAT_HAVE_PV_GUEST_ENTRY 1
>  
> +#define XEN_HAVE_PV_UPCALL_MASK 1
> +#define COMPAT_HAVE_PV_UPCALL_MASK 1

Same here - if needed at all, it ought to start with XEN_. But by
now I'm pretty convinced that this isn't needed in the public
headers.

Jan

Re: [PATCH v5 0/5] xen: public interface and foreign struct check changes for arm

[Jan Beulich]

>>> On 18.07.13 at 15:15, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> I last posted this back in April to critical acclaim (AKA near total
> silence).
> 
> I'm not sure who looks after tools/include/xen-foreign. I had thought it
> was Jan but I think I was confused and was thinking of the semi-related
> xen/include/compat stuff. IOW I think nobody felt "responsible".

Indeed, most of it seemed (wrongly to a degree) to be tools stuff,
and hence I skipped it. Apologies.

> Unless there's any objection lets just treat this as coming under tools.
> The alternative is that since it is related to hypervisor ABI it comes
> under general hypervisor stuff.

The latter, at least for everything that's under xen/.

Jan

Re: [PATCH v5 0/5] xen: public interface and foreign struct check changes for arm

[Ian Campbell]

On Thu, 2013-07-18 at 14:42 +0100, Jan Beulich wrote:
> >>> On 18.07.13 at 15:15, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > I last posted this back in April to critical acclaim (AKA near total
> > silence).
> > 
> > I'm not sure who looks after tools/include/xen-foreign. I had thought it
> > was Jan but I think I was confused and was thinking of the semi-related
> > xen/include/compat stuff. IOW I think nobody felt "responsible".
> 
> Indeed, most of it seemed (wrongly to a degree) to be tools stuff,
> and hence I skipped it. Apologies.
> 
> > Unless there's any objection lets just treat this as coming under tools.
> > The alternative is that since it is related to hypervisor ABI it comes
> > under general hypervisor stuff.
> 
> The latter, at least for everything that's under xen/.

Right, by "this" I meant only the xen-foreign bits, sorry I wsn't clear
about that.

> 
> Jan
> 

Re: [PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Ian Campbell]

On Thu, 2013-07-18 at 14:38 +0100, Jan Beulich wrote:
> >>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
> > --- a/xen/include/public/arch-x86/xen.h
> > +++ b/xen/include/public/arch-x86/xen.h
> > @@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
> >  #define PRI_xen_pfn "lx"
> >  #endif
> >  
> > +#define XEN_HAVE_PV_GUEST_ENTRY 1
> > +#define COMPAT_HAVE_PV_GUEST_ENTRY 1
> 
> This ought to nevertheless have a XEN_ prefix, if it really is
> needed at all (didn't spot yet where it's being consumed).

It gets used in the autogenerated xen/include/compat/xen.h:
        ifdef COMPAT_HAVE_PV_GUEST_ENTRY
        struct compat_start_info {
        ...

I think the COMPAT prefix is consistent with other aspects of this
autogeneration (in particular the struct renaming xen_xxx -> compat_xxx)

Same applies to 5/5.

Ian.

Re: [PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Jan Beulich]

>>> On 18.07.13 at 15:57, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2013-07-18 at 14:38 +0100, Jan Beulich wrote:
>> >>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > --- a/xen/include/public/arch-x86/xen.h
>> > +++ b/xen/include/public/arch-x86/xen.h
>> > @@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
>> >  #define PRI_xen_pfn "lx"
>> >  #endif
>> >  
>> > +#define XEN_HAVE_PV_GUEST_ENTRY 1
>> > +#define COMPAT_HAVE_PV_GUEST_ENTRY 1
>> 
>> This ought to nevertheless have a XEN_ prefix, if it really is
>> needed at all (didn't spot yet where it's being consumed).
> 
> It gets used in the autogenerated xen/include/compat/xen.h:
>         ifdef COMPAT_HAVE_PV_GUEST_ENTRY
>         struct compat_start_info {
>         ...
> 
> I think the COMPAT prefix is consistent with other aspects of this
> autogeneration (in particular the struct renaming xen_xxx -> compat_xxx)

Yeah, except that these auto-generated pieces are part of the
public interface. I don't mind the definitions getting added, what
I do mind is them getting into the public headers.

Jan

Re: [PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Ian Campbell]

On Thu, 2013-07-18 at 15:04 +0100, Jan Beulich wrote:
> >>> On 18.07.13 at 15:57, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2013-07-18 at 14:38 +0100, Jan Beulich wrote:
> >> >>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > --- a/xen/include/public/arch-x86/xen.h
> >> > +++ b/xen/include/public/arch-x86/xen.h
> >> > @@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
> >> >  #define PRI_xen_pfn "lx"
> >> >  #endif
> >> >  
> >> > +#define XEN_HAVE_PV_GUEST_ENTRY 1
> >> > +#define COMPAT_HAVE_PV_GUEST_ENTRY 1
> >> 
> >> This ought to nevertheless have a XEN_ prefix, if it really is
> >> needed at all (didn't spot yet where it's being consumed).
> > 
> > It gets used in the autogenerated xen/include/compat/xen.h:
> >         ifdef COMPAT_HAVE_PV_GUEST_ENTRY
> >         struct compat_start_info {
> >         ...
> > 
> > I think the COMPAT prefix is consistent with other aspects of this
> > autogeneration (in particular the struct renaming xen_xxx -> compat_xxx)
> 
> Yeah, except that these auto-generated pieces are part of the
> public interface. I don't mind the definitions getting added, what
> I do mind is them getting into the public headers.

So where should they go instead?

> 
> Jan
> 

Re: [PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Jan Beulich]

>>> On 18.07.13 at 16:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Thu, 2013-07-18 at 15:04 +0100, Jan Beulich wrote:
>> >>> On 18.07.13 at 15:57, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Thu, 2013-07-18 at 14:38 +0100, Jan Beulich wrote:
>> >> >>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
>> >> > --- a/xen/include/public/arch-x86/xen.h
>> >> > +++ b/xen/include/public/arch-x86/xen.h
>> >> > @@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
>> >> >  #define PRI_xen_pfn "lx"
>> >> >  #endif
>> >> >  
>> >> > +#define XEN_HAVE_PV_GUEST_ENTRY 1
>> >> > +#define COMPAT_HAVE_PV_GUEST_ENTRY 1
>> >> 
>> >> This ought to nevertheless have a XEN_ prefix, if it really is
>> >> needed at all (didn't spot yet where it's being consumed).
>> > 
>> > It gets used in the autogenerated xen/include/compat/xen.h:
>> >         ifdef COMPAT_HAVE_PV_GUEST_ENTRY
>> >         struct compat_start_info {
>> >         ...
>> > 
>> > I think the COMPAT prefix is consistent with other aspects of this
>> > autogeneration (in particular the struct renaming xen_xxx -> compat_xxx)
>> 
>> Yeah, except that these auto-generated pieces are part of the
>> public interface. I don't mind the definitions getting added, what
>> I do mind is them getting into the public headers.
> 
> So where should they go instead?

Perhaps into asm-x86/config.h, which already has a couple of
other COMPAT_* ones derived from public XEN_* ones.

And perhaps the COMPAT_* definition should then also simply
use the XEN_* one as its expansion, rather than saying "1"
literally.

Jan

Re: [PATCH 2/5] xen: only expose start_info on architectures which have a PV boot path

[Ian Campbell]

On Thu, 2013-07-18 at 15:12 +0100, Jan Beulich wrote:
> >>> On 18.07.13 at 16:08, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Thu, 2013-07-18 at 15:04 +0100, Jan Beulich wrote:
> >> >>> On 18.07.13 at 15:57, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> > On Thu, 2013-07-18 at 14:38 +0100, Jan Beulich wrote:
> >> >> >>> On 18.07.13 at 15:15, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> >> > --- a/xen/include/public/arch-x86/xen.h
> >> >> > +++ b/xen/include/public/arch-x86/xen.h
> >> >> > @@ -70,6 +70,9 @@ typedef unsigned long xen_pfn_t;
> >> >> >  #define PRI_xen_pfn "lx"
> >> >> >  #endif
> >> >> >  
> >> >> > +#define XEN_HAVE_PV_GUEST_ENTRY 1
> >> >> > +#define COMPAT_HAVE_PV_GUEST_ENTRY 1
> >> >> 
> >> >> This ought to nevertheless have a XEN_ prefix, if it really is
> >> >> needed at all (didn't spot yet where it's being consumed).
> >> > 
> >> > It gets used in the autogenerated xen/include/compat/xen.h:
> >> >         ifdef COMPAT_HAVE_PV_GUEST_ENTRY
> >> >         struct compat_start_info {
> >> >         ...
> >> > 
> >> > I think the COMPAT prefix is consistent with other aspects of this
> >> > autogeneration (in particular the struct renaming xen_xxx -> compat_xxx)
> >> 
> >> Yeah, except that these auto-generated pieces are part of the
> >> public interface. I don't mind the definitions getting added, what
> >> I do mind is them getting into the public headers.
> > 
> > So where should they go instead?
> 
> Perhaps into asm-x86/config.h, which already has a couple of
> other COMPAT_* ones derived from public XEN_* ones.
> 
> And perhaps the COMPAT_* definition should then also simply
> use the XEN_* one as its expansion, rather than saying "1"
> literally.

Sounds good. I've noticed one other fixlet which I need (to 5/5) so I'll
resend with that change too.

Ian.

Re: [PATCH 5/5] xen: remove evtchn_upcall_mask from interface on ARM

[Ian Campbell]

On Thu, 2013-07-18 at 14:15 +0100, Ian Campbell wrote:
> On ARM event-channel upcalls are masked using the hardware's interrupt mask
> bit and not by a software bit.
> 
> Leaving this field present in the interface has caused some confusion already
> and is liable to mean it gets inadvertently used in the future. So arrange for
> this field to be turned into a padding field on ARM by introducing a
> XEN_HAVE_PV_UPCALL_MASK define.
> 
> This bit is also unused for x86 PV-on-HVM guests, but we can't realistically
> distinguish those from x86 PV guests in the headers.
> 
> Add a per-arch vcpu_event_delivery_is_enabled function to replace the single
> open coded use of evtchn_upcall_mask in common code (in a debug keyhandler).
> The existing local_event_delivery_is_enabled, which operates only on current,
> was unimplemented on ARM and unused on x86, so remove it.

I failed to notice the new use of this in common code which arose from
moving map_vcpu_info to common code.

The most obvious fix is:

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 6c264a5..9390a22 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -921,7 +921,9 @@ int map_vcpu_info(struct vcpu *v, unsigned long gfn, unsigned offset)
     if ( v->vcpu_info == &dummy_vcpu_info )
     {
         memset(new_info, 0, sizeof(*new_info));
+#ifdef XEN_HAVE_PV_UPCALL_MASK
         __vcpu_info(v, new_info, evtchn_upcall_mask) = 1;
+#endif
     }
     else
     {

But that doesn't feel very satisfactory.

Any bright ideas? arch_vcpu_info_init(new_info) perhaps?

Ian.