[PATCH 0/2] Allow chnaging of external varibles in the hypervisor

[PATCH 0/2] Allow chnaging of external varibles in the hypervisor

[Don Slutz]

The 1st patch just skips calling rcu_lock_domain_by_id() for
XEN_DOMCTL_gdbsx_guestmemio.

The code that handles XEN_DOMCTL_gdbsx_guestmemio, does not use d
and has it's own domain checking code.

The 2nd patch is an example tool thats uses
XEN_DOMCTL_gdbsx_guestmemio to read and modify the Hymbervisor's
memeory.

Don Slutz (2):
  gdbsx_guestmemio: Allow it to check domain.
  Add xen-hyp-rw

 .gitignore                          |   1 +
 tools/debugger/gdbsx/Makefile       |   7 +-
 tools/debugger/gdbsx/xen-hyp-rw.c   | 209 ++++++++++++++++++++++++++++++++++++
 tools/debugger/gdbsx/xg/xg_main.c   |  28 +++++
 tools/debugger/gdbsx/xg/xg_public.h |   2 +
 xen/common/domctl.c                 |   1 +
 6 files changed, 247 insertions(+), 1 deletion(-)
 create mode 100644 tools/debugger/gdbsx/xen-hyp-rw.c

-- 
1.8.4

[PATCH 1/2] gdbsx_guestmemio: Allow it to check domain.

[Don Slutz]

gdbsx_guest_mem_io() does not get d passed, it expects to handle
the domain lookup itself.

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
 xen/common/domctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index ce517a7..2a2d203 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -443,6 +443,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
     case XEN_DOMCTL_createdomain:
     case XEN_DOMCTL_getdomaininfo:
     case XEN_DOMCTL_test_assign_device:
+    case XEN_DOMCTL_gdbsx_guestmemio:
         d = NULL;
         break;
     default:
-- 
1.8.4

Re: [PATCH 1/2] gdbsx_guestmemio: Allow it to check domain.

[Andrew Cooper]

On 18/06/15 16:55, Don Slutz wrote:
> gdbsx_guest_mem_io() does not get d passed, it expects to handle
> the domain lookup itself.
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <don.slutz@gmail.com>

As for the change itself, Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

However, I think the commit message needs improving.  Specifically, the
caller of XEN_DOMCTL_gdbsx_guestmemio is expected to use DOMID_IDLE to
interact with the hypervisor, rather than a domain, which doesn't
interact well with with the domain rcu lock.

> ---
>  xen/common/domctl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
> index ce517a7..2a2d203 100644
> --- a/xen/common/domctl.c
> +++ b/xen/common/domctl.c
> @@ -443,6 +443,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>      case XEN_DOMCTL_createdomain:
>      case XEN_DOMCTL_getdomaininfo:
>      case XEN_DOMCTL_test_assign_device:
> +    case XEN_DOMCTL_gdbsx_guestmemio:
>          d = NULL;
>          break;
>      default:

Re: [PATCH 1/2] gdbsx_guestmemio: Allow it to check domain.

[Don Slutz]

On 06/18/15 12:59, Andrew Cooper wrote:
> On 18/06/15 16:55, Don Slutz wrote:
>> gdbsx_guest_mem_io() does not get d passed, it expects to handle
>> the domain lookup itself.
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <don.slutz@gmail.com>
> 
> As for the change itself, Reviewed-by: Andrew Cooper
> <andrew.cooper3@citrix.com>
> 
> However, I think the commit message needs improving.  Specifically, the
> caller of XEN_DOMCTL_gdbsx_guestmemio is expected to use DOMID_IDLE to
> interact with the hypervisor, rather than a domain, which doesn't
> interact well with with the domain rcu lock.

I am happy to change it to:

Specifically, the caller of XEN_DOMCTL_gdbsx_guestmemio is expected to
use DOMID_IDLE to interact with the hypervisor, rather than a domain,
which doesn't interact well with with the domain rcu lock.

Do you want me to add:

(h/t to Andrew Cooper for the patch summary.)

  -Don Slutz

> 
>> ---
>>  xen/common/domctl.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/xen/common/domctl.c b/xen/common/domctl.c
>> index ce517a7..2a2d203 100644
>> --- a/xen/common/domctl.c
>> +++ b/xen/common/domctl.c
>> @@ -443,6 +443,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
>>      case XEN_DOMCTL_createdomain:
>>      case XEN_DOMCTL_getdomaininfo:
>>      case XEN_DOMCTL_test_assign_device:
>> +    case XEN_DOMCTL_gdbsx_guestmemio:
>>          d = NULL;
>>          break;
>>      default:
> 

[PATCH 2/2] Add xen-hyp-rw

[Don Slutz]

This allows reading and writing of variables in the hypervisor.

for example (read case -- default 4 bytes):

  xen-hyp-rw /boot/System.map-xen* opt_hvm_debug_level
  opt_hvm_debug_level @ 0xffff82d080285610 is 0x0(0)

Write case:

  xen-hyp-rw /boot/System.map-xen* opt_hvm_debug_level 4 -1
  opt_hvm_debug_level @ 0xffff82d080285610 is 0x0(0)
  opt_hvm_debug_level @ 0xffff82d080285610 set to 0xffffffff(4294967295)

Signed-off-by: Don Slutz <dslutz@verizon.com>
CC: Don Slutz <don.slutz@gmail.com>
---
 .gitignore                          |   1 +
 tools/debugger/gdbsx/Makefile       |   7 +-
 tools/debugger/gdbsx/xen-hyp-rw.c   | 209 ++++++++++++++++++++++++++++++++++++
 tools/debugger/gdbsx/xg/xg_main.c   |  28 +++++
 tools/debugger/gdbsx/xg/xg_public.h |   2 +
 5 files changed, 246 insertions(+), 1 deletion(-)
 create mode 100644 tools/debugger/gdbsx/xen-hyp-rw.c

diff --git a/.gitignore b/.gitignore
index 3f42ded..73dce09 100644
--- a/.gitignore
+++ b/.gitignore
@@ -104,6 +104,7 @@ tools/debugger/gdb/gdb-6.2.1-linux-i386-xen/*
 tools/debugger/gdb/gdb-6.2.1/*
 tools/debugger/gdb/gdb-6.2.1.tar.bz2
 tools/debugger/gdbsx/gdbsx
+tools/debugger/gdbsx/xen-hyp-rw
 tools/debugger/xenitp/xenitp
 tools/firmware/*/biossums
 tools/firmware/*.bin
diff --git a/tools/debugger/gdbsx/Makefile b/tools/debugger/gdbsx/Makefile
index 4ed6d76..6a8c4ac 100644
--- a/tools/debugger/gdbsx/Makefile
+++ b/tools/debugger/gdbsx/Makefile
@@ -6,10 +6,11 @@ all:
 	$(MAKE) -C gx
 	$(MAKE) -C xg
 	$(MAKE) gdbsx
+	$(MAKE) xen-hyp-rw
 
 .PHONY: clean
 clean:
-	rm -f xg_all.a gx_all.a gdbsx
+	rm -f xg_all.a gx_all.a gdbsx xen-hyp-rw
 	set -e; for d in xg gx; do $(MAKE) -C $$d clean; done
 
 .PHONY: distclean
@@ -20,10 +21,14 @@ distclean: clean
 install: all
 	[ -d $(DESTDIR)$(sbindir) ] || $(INSTALL_DIR) $(DESTDIR)$(sbindir)
 	$(INSTALL_PROG) gdbsx $(DESTDIR)$(sbindir)/gdbsx
+	$(INSTALL_PROG) xen-hyp-rw $(DESTDIR)$(sbindir)/xen-hyp-rw
 
 gdbsx: gx/gx_all.a xg/xg_all.a 
 	$(CC) -o $@ $^
 
+xen-hyp-rw: xen-hyp-rw.c gx/gx_all.a xg/xg_all.a
+	$(CC) $(CFLAGS) $(CFLAGS_xeninclude) -o $@ $^
+
 xg/xg_all.a:
 	$(MAKE) -C xg
 gx/gx_all.a:
diff --git a/tools/debugger/gdbsx/xen-hyp-rw.c b/tools/debugger/gdbsx/xen-hyp-rw.c
new file mode 100644
index 0000000..ee10fe9
--- /dev/null
+++ b/tools/debugger/gdbsx/xen-hyp-rw.c
@@ -0,0 +1,209 @@
+/******************************************************************************
+ * tools/debugger/gdbsx/xen-hyp-rw.c
+ *
+ * read and write hypervisor memory.
+ *
+ * Copyright (C) 2014 by Verizon.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <stdint.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <string.h>
+#include <errno.h>
+#include <assert.h>
+#include <signal.h>
+#include <ctype.h>
+
+#include <xen/xen.h>
+
+typedef unsigned char uchar;
+typedef long long unsigned int u64;
+
+#include "xg/xg_public.h"
+
+void gxprt(const char *fmt, ...);
+
+static u64 read_symbol_table(const char *symtab, const char *who)
+{
+    u64 ret = 0;
+    char type, line[256];
+    char *p;
+    FILE *f;
+    u64 address;
+
+    f = fopen(symtab, "r");
+    if ( f == NULL )
+    {
+        fprintf(stderr, "failed to open symbol table %s\n", symtab);
+        exit(-1);
+    }
+
+    while ( !feof(f) )
+    {
+        if ( fgets(line, 256, f) == NULL )
+            break;
+
+        /* need more checks for syntax here... */
+        address = strtoull(line, &p, 16);
+        if ( !isspace((uint8_t)*p++) )
+            continue;
+        type = *p++;
+        if ( !isalpha((uint8_t)type) && type != '?' )
+            continue;
+        if ( !isspace((uint8_t)*p++) )
+            continue;
+
+        /* in the future we should handle the module name
+         * being appended here, this would allow us to use
+         * /proc/kallsyms as our symbol table
+         */
+        if ( p[strlen(p) - 1] == '\n' )
+            p[strlen(p) - 1] = '\0';
+
+        switch ( type )
+        {
+        case 'A': /* global absolute */
+        case 'a': /* local absolute */
+            break;
+        case 'U': /* undefined */
+        case 'v': /* undefined weak object */
+        case 'w': /* undefined weak function */
+            continue;
+        default:
+            break;
+        }
+
+        if ( strcmp(p, who) == 0 )
+            ret = address;
+    }
+
+    fclose(f);
+
+    return ret;
+}
+
+int
+main(int argc, char *argv[])
+{
+    domid_t domid = DOMID_IDLE;
+    int exit_rc = 0;
+    u64 hyp_va;
+    u64 val = 0;
+    long len = sizeof(int);
+    char *endptr = NULL;
+    int remain;
+
+    if ( strcmp(argv[argc - 1], "-d") == 0 )
+    {
+        xgtrc_on = 1;       /* debug trace on */
+        argc--;
+    }
+
+    if ( argc < 3 )
+    {
+        printf("Usage: %s [-d] <symbol file> <symbol> [<length> [<new value>]]\n",
+               argv[0]);
+        gxprt("ERROR: Need symbol file and symbol\n");
+        exit(1);
+    }
+
+    hyp_va = read_symbol_table(argv[1], argv[2]);
+    if ( !hyp_va )
+    {
+        gxprt("ERROR: failed to find symbol:%s\n", argv[2]);
+        exit(1);
+    }
+
+    if ( argc > 3 )
+    {
+        errno = 0;
+        len = strtol(argv[3], &endptr, 0);
+        if ( endptr == argv[3] ||
+             *endptr )
+        {
+            gxprt("ERROR: Failed to convert '%s' to a long.\n",
+                  argv[3]);
+            exit(2);
+        }
+        if ( len < 1 )
+        {
+            gxprt("ERROR: length=%ld too small.\n", len);
+            exit(3);
+        }
+        else if ( len > sizeof(val) )
+        {
+            gxprt("ERROR: length=%ld too big.\n", len);
+            exit(4);
+        }
+    }
+
+    if ( xg_init() == -1 )
+    {
+        gxprt("ERROR: failed to initialize errno:%d\n", errno);
+        exit(1);
+    }
+    if ( (hyp_attach(domid)) == -1 )
+    {
+        gxprt("ERROR: failed to attach to domain:%d errno:%d\n",
+              domid, errno);
+        exit(1);
+    }
+    if ( (remain = xg_read_mem(hyp_va, (char *)&val, len, 0) != 0) )
+        XGERR("Failed read mem. addr:0x%llx len:%d remn:%d errno:%d\n",
+              hyp_va, len, remain, errno);
+    else
+    {
+        printf("%s @ 0x%llx is 0x%llx(%lld)\n",
+               argv[2], hyp_va, val, val);
+        if ( argc > 4 )
+        {
+            u64 new = strtoll(argv[4], &endptr, 0);
+
+            if ( endptr && (*endptr == 0) )
+            {
+                if ( (remain = xg_write_mem(hyp_va, (char *)&new, len, 0)
+                      != 0) )
+                    XGERR(
+                        "Failed write addr:0x%llx len:%d remn:%d errno:%d\n",
+                        hyp_va, len, remain, errno);
+                else
+                {
+                    if ( (remain = xg_read_mem(hyp_va, (char *)&val, len, 0)
+                          != 0) )
+                        XGERR(
+                            "Failed read addr:0x%llx len:%d remn:%d errno:%d\n",
+                            hyp_va, len, remain, errno);
+                    printf("%s @ 0x%llx set to 0x%llx(%lld)\n",
+                           argv[2], hyp_va, val, val);
+                }
+            }
+        }
+    }
+
+    hyp_detach();
+    return exit_rc;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/debugger/gdbsx/xg/xg_main.c b/tools/debugger/gdbsx/xg/xg_main.c
index c95e4ed..32e16a4 100644
--- a/tools/debugger/gdbsx/xg/xg_main.c
+++ b/tools/debugger/gdbsx/xg/xg_main.c
@@ -340,6 +340,7 @@ xg_detach_deinit(void)
         _unpause_domain();
         
     close(_dom0_fd);
+    munlock(&domctl, sizeof(domctl));
 }
 
 /* 
@@ -829,6 +830,33 @@ xg_write_mem(uint64_t guestva, char *frombuf, int buflen, uint64_t pgd3val)
 }
 
 /*
+ * Attach to xen for debugging.
+ */
+int
+hyp_attach(int domid)
+{
+    XGTRC("E:domid:%d\n", domid);
+
+    _dom_id = domctl.domain = domid;
+    domctl.interface_version = XEN_DOMCTL_INTERFACE_VERSION;
+
+    if (mlock(&domctl, sizeof(domctl))) {
+        XGERR("Unable to pin domctl in memory. errno:%d\n", errno);
+        return -1;
+    }
+
+    return 0;
+}
+
+/* Detach from xen for debugger exit */
+void
+hyp_detach(void)
+{
+    close(_dom0_fd);
+    munlock(&domctl, sizeof(domctl));
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/tools/debugger/gdbsx/xg/xg_public.h b/tools/debugger/gdbsx/xg/xg_public.h
index 6236d08..976cf5e 100644
--- a/tools/debugger/gdbsx/xg/xg_public.h
+++ b/tools/debugger/gdbsx/xg/xg_public.h
@@ -108,3 +108,5 @@ int xg_read_mem(uint64_t, char *, int, uint64_t);
 int xg_write_mem(uint64_t, char *, int, uint64_t);
 void xgprt(const char *fn, const char *fmt, ...);
 void xgtrc(const char *fn, const char *fmt, ...);
+int hyp_attach(int);
+void hyp_detach(void);
-- 
1.8.4

Re: [PATCH 2/2] Add xen-hyp-rw

[Andrew Cooper]

On 18/06/15 16:55, Don Slutz wrote:
> This allows reading and writing of variables in the hypervisor.
>
> for example (read case -- default 4 bytes):
>
>   xen-hyp-rw /boot/System.map-xen* opt_hvm_debug_level
>   opt_hvm_debug_level @ 0xffff82d080285610 is 0x0(0)
>
> Write case:
>
>   xen-hyp-rw /boot/System.map-xen* opt_hvm_debug_level 4 -1
>   opt_hvm_debug_level @ 0xffff82d080285610 is 0x0(0)
>   opt_hvm_debug_level @ 0xffff82d080285610 set to 0xffffffff(4294967295)
>
> Signed-off-by: Don Slutz <dslutz@verizon.com>
> CC: Don Slutz <don.slutz@gmail.com>

As a crazy thought...

Would it not be easier to modify gdbsx to allow gdb to connect remotely
to the hypervisor context?

That way, any gdb-like tools could consume an appropriate xen-syms and
make changes like this.

~Andrew

Re: [PATCH 2/2] Add xen-hyp-rw

[Don Slutz]

On 06/18/15 13:05, Andrew Cooper wrote:
> On 18/06/15 16:55, Don Slutz wrote:
>> This allows reading and writing of variables in the hypervisor.
>>
>> for example (read case -- default 4 bytes):
>>
>>   xen-hyp-rw /boot/System.map-xen* opt_hvm_debug_level
>>   opt_hvm_debug_level @ 0xffff82d080285610 is 0x0(0)
>>
>> Write case:
>>
>>   xen-hyp-rw /boot/System.map-xen* opt_hvm_debug_level 4 -1
>>   opt_hvm_debug_level @ 0xffff82d080285610 is 0x0(0)
>>   opt_hvm_debug_level @ 0xffff82d080285610 set to 0xffffffff(4294967295)
>>
>> Signed-off-by: Don Slutz <dslutz@verizon.com>
>> CC: Don Slutz <don.slutz@gmail.com>
> 
> As a crazy thought...
> 
> Would it not be easier to modify gdbsx to allow gdb to connect remotely
> to the hypervisor context?

I tried this to start, but kept running into issues.  A lot of them were
about pausing and unpausing the target domain.

> 
> That way, any gdb-like tools could consume an appropriate xen-syms and
> make changes like this.
> 

Yes.  This code was a lot faster to get working.

I used a lot of the xg "library" of gdbsx to do the work.  Only 2 new
routines: hyp_attach() and hyp_detach() in that code.

   -Don Slutz

> ~Andrew
>