Re: [Qemu-devel] [PATCH v6.1 00/11] xen: xen-domid-restrict improvements

Re: [Qemu-devel] [PATCH v6.1 00/11] xen: xen-domid-restrict improvements

[Ian Jackson]

This just-amended series provides necessary support for running qemu
as a Xen device model without power equivalent to root.  In
particular, it makes -xen-domid-restrict effective.

Compared to v6, it fixes some minor checkpatch, style and portability
issues.  Sorry for the earlier troubled submission.

Compared to v5, I have taken into account all the comments from v5
(from October!) and there are also two new patches from Ross
Lagerwall.

 +.    [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of     
 m   a [PATCH 02/12] xen: link against xentoolcore                            
     a [PATCH 03/12] xen: restrict: use xentoolcore_restrict_all              
    r  [PATCH 04/12] xen: defer call to xen_restrict until just before        
     a [PATCH 05/12] xen: destroy_hvm_domain: Move reason into a variable     
    ra [PATCH 06/12] xen: move xc_interface compatibility fallback further    
 *. r  [PATCH 07/12] xen: destroy_hvm_domain: Try xendevicemodel_shutdown     
 *. r  [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility        
 m     [PATCH 09/12] configure: do_compiler: Dump some extra info under bash  
 +.    [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory        
 +     [PATCH 11/12] xen: Expect xenstore write to fail when restricted       
 +     [PATCH 12/12] scripts/get_maintainer.pl: Print proper error message    

 m = commit message (only) changed in v6.1 of the series (compared to v5)
 * = patch changed in v6.1 of the series (compared to v5)
 + = new patch
 . = changes made in v6.1 (compared to v6)
 r = reviewed (by someone other than me)
 a = acked

Thanks for your attention.

Regards,
Ian.

[Qemu-devel] [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types

[Ian Jackson]

This avoids checkpatch misparsing (as statements) long function
definitions or declarations, which sometimes start with constructs
like this:

  static inline int xendevicemodel_relocate_memory(
      xendevicemodel_handle *dmod, domid_t domid, ...

CC: Eric Blake <eblake@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
v6.1: New patch
---
 scripts/checkpatch.pl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d1fe79b..3e488f7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -266,6 +266,7 @@ our @typeList = (
 	qr{target_(?:u)?long},
 	qr{hwaddr},
 	qr{xml${Ident}},
+	qr{xendevicemodel_handle},
 );
 
 # This can be modified by sub possible.  Since it can be empty, be careful
-- 
2.1.4

Re: [Qemu-devel] [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types

[Paolo Bonzini]

On 08/03/2018 20:02, Ian Jackson wrote:
> This avoids checkpatch misparsing (as statements) long function
> definitions or declarations, which sometimes start with constructs
> like this:
> 
>   static inline int xendevicemodel_relocate_memory(
>       xendevicemodel_handle *dmod, domid_t domid, ...
> 
> CC: Eric Blake <eblake@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
> v6.1: New patch
> ---
>  scripts/checkpatch.pl | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d1fe79b..3e488f7 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -266,6 +266,7 @@ our @typeList = (
>  	qr{target_(?:u)?long},
>  	qr{hwaddr},
>  	qr{xml${Ident}},
> +	qr{xendevicemodel_handle},
>  );
>  
>  # This can be modified by sub possible.  Since it can be empty, be careful
> -- 

Or just rename it so that it is CamelCase.  Then checkpatch will be happy.

Paolo

Re: [Qemu-devel] [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types

[Ian Jackson]

Paolo Bonzini writes ("Re: [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types"):
> On 08/03/2018 20:02, Ian Jackson wrote:
> > This avoids checkpatch misparsing (as statements) long function
> > definitions or declarations, which sometimes start with constructs
> > like this:
> > 
> >   static inline int xendevicemodel_relocate_memory(
> >       xendevicemodel_handle *dmod, domid_t domid, ...
...
> 
> Or just rename it so that it is CamelCase.  Then checkpatch will be happy.

I can do that if you want, although it seems a bit like pointless
churn.  If I do that it ought to be renamed as well as re-spelled
because xendevicemodel_handle is actually a struct, not a "handle"
(ie, not a scalar).[1]  So
   s/xendevicemodel_handle/XendevicemodelInterface/
I guess ?

[1] If it were a scalar CODING_STYLE says it should be in lowercase
with _t.  This is a violation of the C standard, which reserves names
ending _t for the implementation...

Ian.

Re: [Qemu-devel] [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types"):
> Paolo Bonzini writes ("Re: [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types"):
> > Or just rename it so that it is CamelCase.  Then checkpatch will be happy.
> 
> I can do that if you want,

No I can't, because it's from the Xen headers, not a qemu type.

I have added a note to the commit message for the checkpatch commit.

Ian.

[Qemu-devel] [PATCH 02/12] xen: link against xentoolcore

[Ian Jackson]

From: Anthony PERARD <anthony.perard@citrix.com>

Xen libraries in 4.10 include a new xentoolcore library.  This
contains the xentoolcore_restrict_all function which we are about to
want to use.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v5: More truthful commit message.
---
 configure | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/configure b/configure
index 6f3921c..0a8059f 100755
--- a/configure
+++ b/configure
@@ -2179,7 +2179,7 @@ if test "$xen" != "no" ; then
       $($pkg_config --modversion xencontrol | sed 's/\./ /g') )"
     xen=yes
     xen_pc="xencontrol xenstore xenguest xenforeignmemory xengnttab"
-    xen_pc="$xen_pc xenevtchn xendevicemodel"
+    xen_pc="$xen_pc xenevtchn xendevicemodel xentoolcore"
     QEMU_CFLAGS="$QEMU_CFLAGS $($pkg_config --cflags $xen_pc)"
     libs_softmmu="$($pkg_config --libs $xen_pc) $libs_softmmu"
     LDFLAGS="$($pkg_config --libs $xen_pc) $LDFLAGS"
@@ -2211,18 +2211,20 @@ EOF
         cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
+#include <xentoolcore.h>
 int main(void) {
   xenforeignmemory_handle *xfmem;
 
   xfmem = xenforeignmemory_open(0, 0);
   xenforeignmemory_map2(xfmem, 0, 0, 0, 0, 0, 0, 0);
+  xentoolcore_restrict_all(0);
 
   return 0;
 }
 EOF
-        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs"
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
       then
-      xen_stable_libs="-lxendevicemodel $xen_stable_libs"
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
       xen_ctrl_version=41000
       xen=yes
     elif
-- 
2.1.4

[Qemu-devel] [PATCH 03/12] xen: restrict: use xentoolcore_restrict_all

[Ian Jackson]

And insist that it works.

Drop individual use of xendevicemodel_restrict and
xenforeignmemory_restrict.  These are not actually effective in this
version of qemu, because qemu has a large number of fds open onto
various Xen control devices.

The restriction arrangements are still not right, because the
restriction needs to be done very late - after qemu has opened all of
its control fds.

xentoolcore_restrict_all and xentoolcore.h are available in Xen 4.10
and later, only.  Provide a compatibility stub.  And drop the
compatibility stubs for the old functions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: Modify the compatibility code, too.
    Bump this patch ahead of "defer call to xen_restrict until running"
    Retain call to xentoolcore_restrict_all
---
 include/hw/xen/xen_common.h | 46 +++++++++++----------------------------------
 1 file changed, 11 insertions(+), 35 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 64a978e..1766bb9 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -91,6 +91,16 @@ static inline void *xenforeignmemory_map2(xenforeignmemory_handle *h,
     return xenforeignmemory_map(h, dom, prot, pages, arr, err);
 }
 
+static inline int xentoolcore_restrict_all(domid_t domid)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
+
+#include <xentoolcore.h>
+
 #endif
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
@@ -218,20 +228,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-static inline int xendevicemodel_restrict(
-    xendevicemodel_handle *dmod, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
-static inline int xenforeignmemory_restrict(
-    xenforeignmemory_handle *fmem, domid_t domid)
-{
-    errno = ENOTTY;
-    return -1;
-}
-
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
 
 #undef XC_WANT_COMPAT_DEVICEMODEL_API
@@ -290,28 +286,8 @@ static inline int xen_modified_memory(domid_t domid, uint64_t first_pfn,
 static inline int xen_restrict(domid_t domid)
 {
     int rc;
-
-    /* Attempt to restrict devicemodel operations */
-    rc = xendevicemodel_restrict(xen_dmod, domid);
+    rc = xentoolcore_restrict_all(domid);
     trace_xen_domid_restrict(rc ? errno : 0);
-
-    if (rc < 0) {
-        /*
-         * If errno is ENOTTY then restriction is not implemented so
-         * there's no point in trying to restrict other types of
-         * operation, but it should not be treated as a failure.
-         */
-        if (errno == ENOTTY) {
-            return 0;
-        }
-
-        return rc;
-    }
-
-    /* Restrict foreignmemory operations */
-    rc = xenforeignmemory_restrict(xen_fmem, domid);
-    trace_xen_domid_restrict(rc ? errno : 0);
-
     return rc;
 }
 
-- 
2.1.4

[Qemu-devel] [PATCH 04/12] xen: defer call to xen_restrict until just before os_setup_post

[Ian Jackson]

We need to restrict *all* the control fds that qemu opens.  Looking in
/proc/PID/fd shows there are many; their allocation seems scattered
throughout Xen support code in qemu.

We must postpone the restrict call until roughly the same time as qemu
changes its uid, chroots (if applicable), and so on.

There doesn't seem to be an appropriate hook already.  The RunState
change hook fires at different times depending on exactly what mode
qemu is operating in.

And it appears that no-one but the Xen code wants a hook at this phase
of execution.  So, introduce a bare call to a new function
xen_setup_post, just before os_setup_post.  Also provide the
appropriate stub for when Xen compilation is disabled.

We do the restriction before rather than after os_setup_post, because
xen_restrict may need to open /dev/null, and os_setup_post might have
called chroot.

Currently this does not work with migration, because when running as
the Xen device model qemu needs to signal to the toolstack that it is
ready.  It currently does this using xenstore, and for incoming
migration (but not for ordinary startup) that happens after
os_setup_post.

It is correct that this happens late: we want the incoming migration
stream to be processed by a restricted qemu.  The fix for this will be
to do the startup notification a different way, without using
xenstore.  (QMP is probably a reasonable choice.)

So for now this restriction feature cannot be used in conjunction with
migration.  (Note that this is not a regression in this patch, because
previously the -xen-restrict-domid call was, in fact, simply
ineffective!)  We will revisit this in the Xen 4.11 release cycle.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com> (maintainer:X86)
CC: Richard Henderson <rth@twiddle.net> (maintainer:X86)
CC: Eduardo Habkost <ehabkost@redhat.com> (maintainer:X86)
CC: Michael S. Tsirkin <mst@redhat.com> (supporter:PC)
---
v5: Discuss problems with migration startup notification
    in the commit message.
v3: Do xen_setup_post just before, not just after, os_setup_post,
    to improve interaction with chroot.  Thanks to Ross Lagerwall.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c   |  8 --------
 hw/xen/xen-common.c     | 13 +++++++++++++
 include/sysemu/sysemu.h |  2 ++
 stubs/xen-hvm.c         |  5 +++++
 vl.c                    |  1 +
 5 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index f24b7d4..9c3b6b3 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1254,14 +1254,6 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
         goto err;
     }
 
-    if (xen_domid_restrict) {
-        rc = xen_restrict(xen_domid);
-        if (rc < 0) {
-            error_report("failed to restrict: error %d", errno);
-            goto err;
-        }
-    }
-
     xen_create_ioreq_server(xen_domid, &state->ioservid);
 
     state->exit.notify = xen_exit_notifier;
diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index 83099dd..f73b416 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -117,6 +117,19 @@ static void xen_change_state_handler(void *opaque, int running,
     }
 }
 
+void xen_setup_post(void)
+{
+    int rc;
+
+    if (xen_domid_restrict) {
+        rc = xen_restrict(xen_domid);
+        if (rc < 0) {
+            perror("xen: failed to restrict");
+            exit(1);
+        }
+    }
+}
+
 static int xen_init(MachineState *ms)
 {
     xen_xc = xc_interface_open(0, 0, 0);
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index d24ad09..6b785a4 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -92,6 +92,8 @@ void qemu_remove_machine_init_done_notifier(Notifier *notify);
 
 void qemu_announce_self(void);
 
+void xen_setup_post(void);
+
 extern int autostart;
 
 typedef enum {
diff --git a/stubs/xen-hvm.c b/stubs/xen-hvm.c
index 0067bcc..48ca8da 100644
--- a/stubs/xen-hvm.c
+++ b/stubs/xen-hvm.c
@@ -13,6 +13,7 @@
 #include "hw/xen/xen.h"
 #include "exec/memory.h"
 #include "qapi/qapi-commands-misc.h"
+#include "sysemu/sysemu.h"
 
 int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
 {
@@ -61,3 +62,7 @@ void xen_hvm_init(PCMachineState *pcms, MemoryRegion **ram_memory)
 void qmp_xen_set_global_dirty_log(bool enable, Error **errp)
 {
 }
+
+void xen_setup_post(void)
+{
+}
diff --git a/vl.c b/vl.c
index dae986b..e6e8e1e 100644
--- a/vl.c
+++ b/vl.c
@@ -4719,6 +4719,7 @@ int main(int argc, char **argv, char **envp)
         vm_start();
     }
 
+    xen_setup_post();
     os_setup_post();
 
     main_loop();
-- 
2.1.4

[Qemu-devel] [PATCH 05/12] xen: destroy_hvm_domain: Move reason into a variable

[Ian Jackson]

We are going to want to reuse this.

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
 hw/i386/xen/xen-hvm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 9c3b6b3..3590d99 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1387,12 +1387,13 @@ void destroy_hvm_domain(bool reboot)
     xc_interface *xc_handle;
     int sts;
 
+    unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
     } else {
-        sts = xc_domain_shutdown(xc_handle, xen_domid,
-                                 reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff);
+        sts = xc_domain_shutdown(xc_handle, xen_domid, reason);
         if (sts != 0) {
             fprintf(stderr, "xc_domain_shutdown failed to issue %s, "
                     "sts %d, %s\n", reboot ? "reboot" : "poweroff",
-- 
2.1.4

[Qemu-devel] [PATCH 06/12] xen: move xc_interface compatibility fallback further up the file

[Ian Jackson]

We are going to want to use the dummy xendevicemodel_handle type in
new stub functions in the CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
section.  So we need to provide that definition, or (as applicable)
include the appropriate header, earlier in the file.

(Ideally the newer compatibility layers would be at the bottom of the
file, so that they can naturally benefit from the compatibility layers
for earlier version.  But that's rather too much for this series.)

No functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
---
v2: New patch in v2 of the series
---
 include/hw/xen/xen_common.h | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 1766bb9..60c4ebb 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -78,6 +78,17 @@ static inline void *xenforeignmemory_map(xc_interface *h, uint32_t dom,
 
 extern xenforeignmemory_handle *xen_fmem;
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
+
+typedef xc_interface xendevicemodel_handle;
+
+#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
+
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#include <xendevicemodel.h>
+
+#endif
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
@@ -105,8 +116,6 @@ static inline int xentoolcore_restrict_all(domid_t domid)
 
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40900
 
-typedef xc_interface xendevicemodel_handle;
-
 static inline xendevicemodel_handle *xendevicemodel_open(
     struct xentoollog_logger *logger, unsigned int open_flags)
 {
@@ -228,11 +237,6 @@ static inline int xendevicemodel_set_mem_type(
     return xc_hvm_set_mem_type(dmod, domid, mem_type, first_pfn, nr);
 }
 
-#else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 40900 */
-
-#undef XC_WANT_COMPAT_DEVICEMODEL_API
-#include <xendevicemodel.h>
-
 #endif
 
 extern xendevicemodel_handle *xen_dmod;
-- 
2.1.4

[Qemu-devel] [PATCH 07/12] xen: destroy_hvm_domain: Try xendevicemodel_shutdown

[Ian Jackson]

xc_interface_open etc. is not going to work if we have dropped
privilege, but xendevicemodel_shutdown will if everything is new
enough.

xendevicemodel_shutdown is only availabe in Xen 4.10 and later, so
provide a stub for earlier versions.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
---
v6.1: Fix { } style issue
v6: Do not print message about harmless condition in ENOTTY case.
v2: Add compatibility stub for Xen < 4.10.
    Fix coding style.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 hw/i386/xen/xen-hvm.c       | 12 ++++++++++++
 include/hw/xen/xen_common.h |  7 +++++++
 2 files changed, 19 insertions(+)

diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index 3590d99..fb727bc 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -1386,9 +1386,21 @@ void destroy_hvm_domain(bool reboot)
 {
     xc_interface *xc_handle;
     int sts;
+    int rc;
 
     unsigned int reason = reboot ? SHUTDOWN_reboot : SHUTDOWN_poweroff;
 
+    if (xen_dmod) {
+        rc = xendevicemodel_shutdown(xen_dmod, xen_domid, reason);
+        if (!rc) {
+            return;
+        }
+        if (errno != ENOTTY /* old Xen */) {
+            perror("xendevicemodel_shutdown failed");
+        }
+        /* well, try the old thing then */
+    }
+
     xc_handle = xc_interface_open(0, 0, 0);
     if (xc_handle == NULL) {
         fprintf(stderr, "Cannot acquire xenctrl handle\n");
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 60c4ebb..4bd30a3 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -108,6 +108,13 @@ static inline int xentoolcore_restrict_all(domid_t domid)
     return -1;
 }
 
+static inline int xendevicemodel_shutdown(xendevicemodel_handle *dmod,
+                                          domid_t domid, unsigned int reason)
+{
+    errno = ENOTTY;
+    return -1;
+}
+
 #else /* CONFIG_XEN_CTRL_INTERFACE_VERSION >= 41000 */
 
 #include <xentoolcore.h>
-- 
2.1.4

[Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Ian Jackson]

This allows the caller to specify a uid and gid to use, even if there
is no corresponding password entry.  This will be useful in certain
Xen configurations.

We don't support just -runas <uid> because: (i) deprivileging without
calling setgroups would be ineffective (ii) given only a uid we don't
know what gid we ought to use (since uids may eppear in multiple
passwd file entries with different gids).

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
---
v6.1: Fix constness of qemu_strtoul end pointer parameter.
v6: Use qemu_strtoul for the first strtoul.
    Use error_report rather than fprintf to print usage error message.
    Fix an error message which still referred to . rather than :
v5: Use : rather than . to separate uid from gid
v4: Changed to reuse option -runas
v3: Error messages fixed.  Thanks to Peter Maydell and Ross Lagerwall.
v2: Coding style fixes.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 os-posix.c      | 62 +++++++++++++++++++++++++++++++++++++++++++++++----------
 qemu-options.hx |  3 ++-
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/os-posix.c b/os-posix.c
index b9c2343..214f8fb 100644
--- a/os-posix.c
+++ b/os-posix.c
@@ -42,6 +42,8 @@
 #endif
 
 static struct passwd *user_pwd;
+static uid_t user_uid = (uid_t)-1;
+static gid_t user_gid = (gid_t)-1;
 static const char *chroot_dir;
 static int daemonize;
 static int daemon_pipe;
@@ -127,6 +129,34 @@ void os_set_proc_name(const char *s)
 #endif
 }
 
+
+static bool os_parse_runas_uid_gid(const char *optarg)
+{
+    unsigned long lv;
+    const char *ep;
+    uid_t got_uid;
+    gid_t got_gid;
+    int rc;
+
+    errno = 0;
+    rc = qemu_strtoul(optarg, &ep, 0, &lv);
+    got_uid = lv; /* overflow here is ID in C99 */
+    if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
+        return false;
+    }
+
+    lv = 0;
+    rc = qemu_strtoul(ep + 1, 0, 0, &lv);
+    got_gid = lv; /* overflow here is ID in C99 */
+    if (rc || got_gid != lv || got_gid == (gid_t)-1) {
+        return false;
+    }
+
+    user_uid = got_uid;
+    user_gid = got_gid;
+    return true;
+}
+
 /*
  * Parse OS specific command line options.
  * return 0 if option handled, -1 otherwise
@@ -144,8 +174,8 @@ void os_parse_cmd_args(int index, const char *optarg)
 #endif
     case QEMU_OPTION_runas:
         user_pwd = getpwnam(optarg);
-        if (!user_pwd) {
-            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
+        if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
+            error_report("User doesn't exist (and is not <uid>:<gid>)");
             exit(1);
         }
         break;
@@ -165,18 +195,28 @@ void os_parse_cmd_args(int index, const char *optarg)
 
 static void change_process_uid(void)
 {
-    if (user_pwd) {
-        if (setgid(user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
+    if (user_pwd || user_uid != (uid_t)-1) {
+        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
+        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
+        if (setgid(intended_gid) < 0) {
+            fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
             exit(1);
         }
-        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
-            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
-                    user_pwd->pw_name, user_pwd->pw_gid);
-            exit(1);
+        if (user_pwd) {
+            if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
+                fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
+                        user_pwd->pw_name, user_pwd->pw_gid);
+                exit(1);
+            }
+        } else {
+            if (setgroups(1, &user_gid) < 0) {
+                fprintf(stderr, "Failed to setgroups(1, [%d])",
+                        user_gid);
+                exit(1);
+            }
         }
-        if (setuid(user_pwd->pw_uid) < 0) {
-            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
+        if (setuid(intended_uid) < 0) {
+            fprintf(stderr, "Failed to setuid(%d)\n", intended_uid);
             exit(1);
         }
         if (setuid(0) != -1) {
diff --git a/qemu-options.hx b/qemu-options.hx
index 6585058..211f2a6 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -3763,7 +3763,8 @@ ETEXI
 
 #ifndef _WIN32
 DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
-    "-runas user     change to user id user just before starting the VM\n",
+    "-runas user     change to user id user just before starting the VM\n" \
+    "                user can be numeric uid:gid instead\n",
     QEMU_ARCH_ALL)
 #endif
 STEXI
-- 
2.1.4

Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Markus Armbruster]

Ian Jackson <ian.jackson@eu.citrix.com> writes:

> This allows the caller to specify a uid and gid to use, even if there
> is no corresponding password entry.  This will be useful in certain
> Xen configurations.
>
> We don't support just -runas <uid> because: (i) deprivileging without
> calling setgroups would be ineffective (ii) given only a uid we don't
> know what gid we ought to use (since uids may eppear in multiple
> passwd file entries with different gids).
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Daniel P. Berrange <berrange@redhat.com>
> ---
> v6.1: Fix constness of qemu_strtoul end pointer parameter.
> v6: Use qemu_strtoul for the first strtoul.
>     Use error_report rather than fprintf to print usage error message.
>     Fix an error message which still referred to . rather than :
> v5: Use : rather than . to separate uid from gid
> v4: Changed to reuse option -runas
> v3: Error messages fixed.  Thanks to Peter Maydell and Ross Lagerwall.
> v2: Coding style fixes.
>
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> ---
>  os-posix.c      | 62 +++++++++++++++++++++++++++++++++++++++++++++++----------
>  qemu-options.hx |  3 ++-
>  2 files changed, 53 insertions(+), 12 deletions(-)
>
> diff --git a/os-posix.c b/os-posix.c
> index b9c2343..214f8fb 100644
> --- a/os-posix.c
> +++ b/os-posix.c
> @@ -42,6 +42,8 @@
>  #endif
>  
>  static struct passwd *user_pwd;
> +static uid_t user_uid = (uid_t)-1;
> +static gid_t user_gid = (gid_t)-1;

As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
over @user_uid, @user_gid.  Awkward.

>  static const char *chroot_dir;
>  static int daemonize;
>  static int daemon_pipe;
> @@ -127,6 +129,34 @@ void os_set_proc_name(const char *s)
>  #endif
>  }
>  
> +
> +static bool os_parse_runas_uid_gid(const char *optarg)
> +{
> +    unsigned long lv;
> +    const char *ep;
> +    uid_t got_uid;
> +    gid_t got_gid;
> +    int rc;
> +
> +    errno = 0;
> +    rc = qemu_strtoul(optarg, &ep, 0, &lv);
> +    got_uid = lv; /* overflow here is ID in C99 */
> +    if (rc || *ep != ':' || got_uid != lv || got_uid == (uid_t)-1) {
> +        return false;
> +    }
> +
> +    lv = 0;

Either zero lv before both qemu_strtoul() or neither one.

> +    rc = qemu_strtoul(ep + 1, 0, 0, &lv);
> +    got_gid = lv; /* overflow here is ID in C99 */
> +    if (rc || got_gid != lv || got_gid == (gid_t)-1) {
> +        return false;
> +    }
> +
> +    user_uid = got_uid;
> +    user_gid = got_gid;
> +    return true;
> +}
> +
>  /*
>   * Parse OS specific command line options.
>   * return 0 if option handled, -1 otherwise
> @@ -144,8 +174,8 @@ void os_parse_cmd_args(int index, const char *optarg)
>  #endif
>      case QEMU_OPTION_runas:
>          user_pwd = getpwnam(optarg);
> -        if (!user_pwd) {
> -            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> +        if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> +            error_report("User doesn't exist (and is not <uid>:<gid>)");

The error message no longer includes the offending value.  Intentional?

Note for later: @user_uid and @user_gid get set only when @user_pwd
remains null.

>              exit(1);
>          }
>          break;
> @@ -165,18 +195,28 @@ void os_parse_cmd_args(int index, const char *optarg)
>  
>  static void change_process_uid(void)
>  {
> -    if (user_pwd) {
> -        if (setgid(user_pwd->pw_gid) < 0) {
> -            fprintf(stderr, "Failed to setgid(%d)\n", user_pwd->pw_gid);
> +    if (user_pwd || user_uid != (uid_t)-1) {
> +        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
> +        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
> +        if (setgid(intended_gid) < 0) {
> +            fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);

error_report(), please.  More of the same below.

>              exit(1);
>          }
> -        if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> -            fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> -                    user_pwd->pw_name, user_pwd->pw_gid);
> -            exit(1);
> +        if (user_pwd) {
> +            if (initgroups(user_pwd->pw_name, user_pwd->pw_gid) < 0) {
> +                fprintf(stderr, "Failed to initgroups(\"%s\", %d)\n",
> +                        user_pwd->pw_name, user_pwd->pw_gid);
> +                exit(1);
> +            }
> +        } else {
> +            if (setgroups(1, &user_gid) < 0) {
> +                fprintf(stderr, "Failed to setgroups(1, [%d])",
> +                        user_gid);
> +                exit(1);
> +            }
>          }
> -        if (setuid(user_pwd->pw_uid) < 0) {
> -            fprintf(stderr, "Failed to setuid(%d)\n", user_pwd->pw_uid);
> +        if (setuid(intended_uid) < 0) {
> +            fprintf(stderr, "Failed to setuid(%d)\n", intended_uid);
>              exit(1);
>          }
>          if (setuid(0) != -1) {

This function is the only user of @user_pwd, @user_uid, @user_gid.

Have you considered replacing global @user_pwd by @user_uid, @user_gid
and @user_name?  --runas with numeric uid and gid would leave @user_name
null.

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 6585058..211f2a6 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -3763,7 +3763,8 @@ ETEXI
>  
>  #ifndef _WIN32
>  DEF("runas", HAS_ARG, QEMU_OPTION_runas, \
> -    "-runas user     change to user id user just before starting the VM\n",
> +    "-runas user     change to user id user just before starting the VM\n" \
> +    "                user can be numeric uid:gid instead\n",
>      QEMU_ARCH_ALL)
>  #endif
>  STEXI

Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Ian Jackson]

Thanks for the review.  Taking your comments out of order slightly:

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility"):
> [change_process_uid] is the only user of @user_pwd, @user_uid, @user_gid.
> 
> Have you considered replacing global @user_pwd by @user_uid, @user_gid
> and @user_name?  --runas with numeric uid and gid would leave @user_name
> null.

That would defer the getpwnam from argument parsing to os_setup_post.
I think that's undesriable.

> Ian Jackson <ian.jackson@eu.citrix.com> writes:
> >  static struct passwd *user_pwd;
> > +static uid_t user_uid = (uid_t)-1;
> > +static gid_t user_gid = (gid_t)-1;
> 
> As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
> over @user_uid, @user_gid.  Awkward.

My patch has the right behaviour: each -runas completely overrides the
previous one.  -runas that sets user_{uid,gid} always clears user_pwd
on the way.  So user_pwd can only be set if the most recent -runas was
a name, and then we should honour the name.

This is rather obscure.  I think you are right that this is confusing.
It ought to be clearer.

I will
  - add a comment next to these three variables saying they must
    all be set at the same time
  - explicitly (redundantly) clear user_pwd in os_parse_runas_uid_gid
  - explicitly set user_{uid,gid} to -1 when -runas gets a
    success from getpwnam
  - assert in change_process_uid that the combination is legal

> > +    errno = 0;
> > +    rc = qemu_strtoul(optarg, &ep, 0, &lv);
...
> > +    lv = 0;
> 
> Either zero lv before both qemu_strtoul() or neither one.

This is a hangover from the previous version which used raw strtoul.
The assignments to both lv and errno are redundant.

> > -        if (!user_pwd) {
> > -            fprintf(stderr, "User \"%s\" doesn't exist\n", optarg);
> > +        if (!user_pwd && !os_parse_runas_uid_gid(optarg)) {
> > +            error_report("User doesn't exist (and is not <uid>:<gid>)");
> 
> The error message no longer includes the offending value.  Intentional?

I was in two minds.  I will put it back.

> > +    if (user_pwd || user_uid != (uid_t)-1) {
> > +        gid_t intended_gid = user_pwd ? user_pwd->pw_gid : user_gid;
> > +        uid_t intended_uid = user_pwd ? user_pwd->pw_uid : user_uid;
> > +        if (setgid(intended_gid) < 0) {
> > +            fprintf(stderr, "Failed to setgid(%d)\n", intended_gid);
> 
> error_report(), please.  More of the same below.

I was following the existing code in this function.  I'll add a
pre-patch to fix this up here, and maybe a post-patch to fix the rest
of this file.

Thanks,
Ian.

Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Markus Armbruster]

Ian Jackson <ian.jackson@citrix.com> writes:

> Thanks for the review.  Taking your comments out of order slightly:
>
> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility"):
>> [change_process_uid] is the only user of @user_pwd, @user_uid, @user_gid.
>> 
>> Have you considered replacing global @user_pwd by @user_uid, @user_gid
>> and @user_name?  --runas with numeric uid and gid would leave @user_name
>> null.
>
> That would defer the getpwnam from argument parsing to os_setup_post.
> I think that's undesriable.

No argument.  But why can't os_parse_cmd_args() call getpwnam() as it
does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
user_pwd?  Store a null name when it parses the argument as UID:GID.

>> Ian Jackson <ian.jackson@eu.citrix.com> writes:
>> >  static struct passwd *user_pwd;
>> > +static uid_t user_uid = (uid_t)-1;
>> > +static gid_t user_gid = (gid_t)-1;
>> 
>> As we'll see below, @user_pwd->pw_uid, @user_pwd_pw_gid take precedence
>> over @user_uid, @user_gid.  Awkward.
>
> My patch has the right behaviour: each -runas completely overrides the
> previous one.  -runas that sets user_{uid,gid} always clears user_pwd
> on the way.  So user_pwd can only be set if the most recent -runas was
> a name, and then we should honour the name.
>
> This is rather obscure.  I think you are right that this is confusing.
> It ought to be clearer.
>
> I will
>   - add a comment next to these three variables saying they must
>     all be set at the same time
>   - explicitly (redundantly) clear user_pwd in os_parse_runas_uid_gid
>   - explicitly set user_{uid,gid} to -1 when -runas gets a
>     success from getpwnam
>   - assert in change_process_uid that the combination is legal

Yes, that's better.  But perhaps you like my idea above.

[...]

Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Ian Jackson]

Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility"):
> Ian Jackson <ian.jackson@citrix.com> writes:
> > That would defer the getpwnam from argument parsing to os_setup_post.
> > I think that's undesriable.
> 
> No argument.  But why can't os_parse_cmd_args() call getpwnam() as it
> does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
> user_pwd?  Store a null name when it parses the argument as UID:GID.

Oh, I see.  It seems less obvious to me than what I have done, but I
can do it like that if you like.

Ian.

Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility

[Markus Armbruster]

Ian Jackson <ian.jackson@citrix.com> writes:

> Markus Armbruster writes ("Re: [Qemu-devel] [PATCH 08/12] os-posix: Provide new -runas <uid>:<gid> facility"):
>> Ian Jackson <ian.jackson@citrix.com> writes:
>> > That would defer the getpwnam from argument parsing to os_setup_post.
>> > I think that's undesriable.
>> 
>> No argument.  But why can't os_parse_cmd_args() call getpwnam() as it
>> does now, then store user_pwd->pw_uid, ->pw_gid and ->pw_name instead of
>> user_pwd?  Store a null name when it parses the argument as UID:GID.
>
> Oh, I see.  It seems less obvious to me than what I have done, but I
> can do it like that if you like.

I just wanted to toss out the idea.  Please use your judgenment and do
it the way you like better.

[Qemu-devel] [PATCH 09/12] configure: do_compiler: Dump some extra info under bash

[Ian Jackson]

This makes it much easier to find a particular thing in config.log.

The information may be lacking in other shells, resulting in harmless
empty output.  (This is why we don't use the proper ${FUNCNAME[*]}
array syntax - other shells will choke on that.)

The extra output is only printed if configure is run with bash.  On
systems where /bin/sh is not bash, it is necessary to say bash
./configure to get the extra debug info in the log.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Kent R. Spillner <kspillner@acm.org>
CC: Janosch Frank <frankja@linux.vnet.ibm.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
v6: Fix commit message wording.
v4: No longer tag this patch RFC.
---
 configure | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/configure b/configure
index 0a8059f..841c146 100755
--- a/configure
+++ b/configure
@@ -60,6 +60,10 @@ do_compiler() {
     # is compiler binary to execute.
     local compiler="$1"
     shift
+    echo >>config.log "
+funcs: ${FUNCNAME}
+lines: ${BASH_LINENO}
+files: ${BASH_SOURCE}"
     echo $compiler "$@" >> config.log
     $compiler "$@" >> config.log 2>&1 || return $?
     # Test passed. If this is an --enable-werror build, rerun
-- 
2.1.4

[Qemu-devel] [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory

[Ian Jackson]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Xen unstable (to be in 4.11) has two new dmops, relocate_memory and
pin_memory_cacheattr. Use these to set up the VGA memory, replacing the
previous calls to libxc. This allows the VGA console to work properly
when QEMU is running restricted (-xen-domid-restrict).

Wrapper functions are provided to allow QEMU to work with older versions
of Xen.

Tweak the error handling while making this change:
* Report pin_memory_cacheattr errors.
* Report errors even when DEBUG_HVM is not set. This is useful for
trying to understand why VGA is not working, since otherwise it just
fails silently.
* Fix the return values when an error occurs. The functions now
consistently return -1 and set errno.

CC: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
v6.1: Fix printf formats to match types in error_report messages
      Fix spurious \n in error_report messages
      Fix { } style issue
v6: New patch in this version of the series
---
 configure                   | 19 +++++++++++++++++
 hw/i386/xen/xen-hvm.c       | 50 ++++++++++++++++++++++++---------------------
 include/hw/xen/xen_common.h | 32 +++++++++++++++++++++++++++++
 3 files changed, 78 insertions(+), 23 deletions(-)

diff --git a/configure b/configure
index 841c146..9a83836 100755
--- a/configure
+++ b/configure
@@ -2213,6 +2213,25 @@ EOF
     # Xen unstable
     elif
         cat > $TMPC <<EOF &&
+#undef XC_WANT_COMPAT_DEVICEMODEL_API
+#define __XEN_TOOLS__
+#include <xendevicemodel.h>
+int main(void) {
+  xendevicemodel_handle *xd;
+
+  xd = xendevicemodel_open(0, 0);
+  xendevicemodel_pin_memory_cacheattr(xd, 0, 0, 0, 0);
+
+  return 0;
+}
+EOF
+        compile_prog "" "$xen_libs -lxendevicemodel $xen_stable_libs -lxentoolcore"
+      then
+      xen_stable_libs="-lxendevicemodel $xen_stable_libs -lxentoolcore"
+      xen_ctrl_version=41100
+      xen=yes
+    elif
+        cat > $TMPC <<EOF &&
 #undef XC_WANT_COMPAT_MAP_FOREIGN_API
 #include <xenforeignmemory.h>
 #include <xentoolcore.h>
diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
index fb727bc..caa563b 100644
--- a/hw/i386/xen/xen-hvm.c
+++ b/hw/i386/xen/xen-hvm.c
@@ -347,7 +347,7 @@ static int xen_add_to_physmap(XenIOState *state,
                               MemoryRegion *mr,
                               hwaddr offset_within_region)
 {
-    unsigned long i = 0;
+    unsigned long nr_pages;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     hwaddr pfn, start_gpfn;
@@ -396,22 +396,26 @@ go_physmap:
 
     pfn = phys_offset >> TARGET_PAGE_BITS;
     start_gpfn = start_addr >> TARGET_PAGE_BITS;
-    for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
-        unsigned long idx = pfn + i;
-        xen_pfn_t gpfn = start_gpfn + i;
-
-        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
-        if (rc) {
-            DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
-            return -rc;
-        }
+    nr_pages = size >> TARGET_PAGE_BITS;
+    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, nr_pages, pfn,
+                                        start_gpfn);
+    if (rc) {
+        int saved_errno = errno;
+
+        error_report("relocate_memory %lu pages from GFN %"HWADDR_PRIx
+                     " to GFN %"HWADDR_PRIx" failed: %s",
+                     nr_pages, pfn, start_gpfn, strerror(saved_errno));
+        errno = saved_errno;
+        return -1;
     }
 
-    xc_domain_pin_memory_cacheattr(xen_xc, xen_domid,
+    rc = xendevicemodel_pin_memory_cacheattr(xen_dmod, xen_domid,
                                    start_addr >> TARGET_PAGE_BITS,
                                    (start_addr + size - 1) >> TARGET_PAGE_BITS,
                                    XEN_DOMCTL_MEM_CACHEATTR_WB);
+    if (rc) {
+        error_report("pin_memory_cacheattr failed: %s", strerror(errno));
+    }
     return xen_save_physmap(state, physmap);
 }
 
@@ -419,7 +423,6 @@ static int xen_remove_from_physmap(XenIOState *state,
                                    hwaddr start_addr,
                                    ram_addr_t size)
 {
-    unsigned long i = 0;
     int rc = 0;
     XenPhysmap *physmap = NULL;
     hwaddr phys_offset = 0;
@@ -438,16 +441,17 @@ static int xen_remove_from_physmap(XenIOState *state,
     size >>= TARGET_PAGE_BITS;
     start_addr >>= TARGET_PAGE_BITS;
     phys_offset >>= TARGET_PAGE_BITS;
-    for (i = 0; i < size; i++) {
-        xen_pfn_t idx = start_addr + i;
-        xen_pfn_t gpfn = phys_offset + i;
-
-        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);
-        if (rc) {
-            fprintf(stderr, "add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
-                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
-            return -rc;
-        }
+    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, size, start_addr,
+                                        phys_offset);
+    if (rc) {
+        int saved_errno = errno;
+
+        error_report("relocate_memory "RAM_ADDR_FMT" pages"
+                     " from GFN %"HWADDR_PRIx
+                     " to GFN %"HWADDR_PRIx" failed: %s",
+                     size, start_addr, phys_offset, strerror(saved_errno));
+        errno = saved_errno;
+        return -1;
     }
 
     QLIST_REMOVE(physmap, list);
diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 4bd30a3..2eed6fc 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -89,6 +89,38 @@ typedef xc_interface xendevicemodel_handle;
 
 #endif
 
+#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100
+
+static inline int xendevicemodel_relocate_memory(
+    xendevicemodel_handle *dmod, domid_t domid, uint32_t size, uint64_t src_gfn,
+    uint64_t dst_gfn)
+{
+    uint32_t i;
+    int rc;
+
+    for (i = 0; i < size; i++) {
+        unsigned long idx = src_gfn + i;
+        xen_pfn_t gpfn = dst_gfn + i;
+
+        rc = xc_domain_add_to_physmap(xen_xc, domid, XENMAPSPACE_gmfn, idx,
+                                      gpfn);
+        if (rc) {
+            return rc;
+        }
+    }
+
+    return 0;
+}
+
+static inline int xendevicemodel_pin_memory_cacheattr(
+    xendevicemodel_handle *dmod, domid_t domid, uint64_t start, uint64_t end,
+    uint32_t type)
+{
+    return xc_domain_pin_memory_cacheattr(xen_xc, domid, start, end, type);
+}
+
+#endif /* CONFIG_XEN_CTRL_INTERFACE_VERSION < 41100 */
+
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 41000
 
 #define XEN_COMPAT_PHYSMAP
-- 
2.1.4

Re: [Qemu-devel] [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory

[Anthony PERARD]

On Thu, Mar 08, 2018 at 07:03:05PM +0000, Ian Jackson wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Xen unstable (to be in 4.11) has two new dmops, relocate_memory and
> pin_memory_cacheattr. Use these to set up the VGA memory, replacing the
> previous calls to libxc. This allows the VGA console to work properly
> when QEMU is running restricted (-xen-domid-restrict).
> 
> Wrapper functions are provided to allow QEMU to work with older versions
> of Xen.
> 
> Tweak the error handling while making this change:
> * Report pin_memory_cacheattr errors.
> * Report errors even when DEBUG_HVM is not set. This is useful for
> trying to understand why VGA is not working, since otherwise it just
> fails silently.
> * Fix the return values when an error occurs. The functions now
> consistently return -1 and set errno.
> 
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> v6.1: Fix printf formats to match types in error_report messages
>       Fix spurious \n in error_report messages
>       Fix { } style issue
> v6: New patch in this version of the series
> ---
>  configure                   | 19 +++++++++++++++++
>  hw/i386/xen/xen-hvm.c       | 50 ++++++++++++++++++++++++---------------------
>  include/hw/xen/xen_common.h | 32 +++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 23 deletions(-)
> 
> diff --git a/hw/i386/xen/xen-hvm.c b/hw/i386/xen/xen-hvm.c
> index fb727bc..caa563b 100644
> --- a/hw/i386/xen/xen-hvm.c
> +++ b/hw/i386/xen/xen-hvm.c
> @@ -347,7 +347,7 @@ static int xen_add_to_physmap(XenIOState *state,
>                                MemoryRegion *mr,
>                                hwaddr offset_within_region)
>  {
> -    unsigned long i = 0;
> +    unsigned long nr_pages;
>      int rc = 0;
>      XenPhysmap *physmap = NULL;
>      hwaddr pfn, start_gpfn;
> @@ -396,22 +396,26 @@ go_physmap:
>  
>      pfn = phys_offset >> TARGET_PAGE_BITS;
>      start_gpfn = start_addr >> TARGET_PAGE_BITS;
> -    for (i = 0; i < size >> TARGET_PAGE_BITS; i++) {
> -        unsigned long idx = pfn + i;
> -        xen_pfn_t gpfn = start_gpfn + i;
> -
> -        rc = xen_xc_domain_add_to_physmap(xen_xc, xen_domid, XENMAPSPACE_gmfn, idx, gpfn);

This patch seems to remove the last users of
xen_xc_domain_add_to_physmap(). Can it be remove from xen_common.h?

With that:
Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks.

> -        if (rc) {
> -            DPRINTF("add_to_physmap MFN %"PRI_xen_pfn" to PFN %"
> -                    PRI_xen_pfn" failed: %d (errno: %d)\n", idx, gpfn, rc, errno);
> -            return -rc;
> -        }
> +    nr_pages = size >> TARGET_PAGE_BITS;
> +    rc = xendevicemodel_relocate_memory(xen_dmod, xen_domid, nr_pages, pfn,
> +                                        start_gpfn);
> +    if (rc) {
> +        int saved_errno = errno;
> +
> +        error_report("relocate_memory %lu pages from GFN %"HWADDR_PRIx
> +                     " to GFN %"HWADDR_PRIx" failed: %s",
> +                     nr_pages, pfn, start_gpfn, strerror(saved_errno));
> +        errno = saved_errno;
> +        return -1;
>      }
>  


-- 
Anthony PERARD

Re: [Qemu-devel] [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory

[Ian Jackson]

Anthony PERARD writes ("Re: [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory"):
> This patch seems to remove the last users of
> xen_xc_domain_add_to_physmap(). Can it be remove from xen_common.h?
> 
> With that:
> Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Have added a separate patch for that.

>From 15dedc627cad96301e4015f1f777fcab3906aba7 Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Fri, 27 Oct 2017 11:23:10 +0100
Subject: [PATCH 5/5] scripts/get_maintainer.pl: Print proper error message for
 missing $file

If you pass scripts/get_maintainer.pl the name of a FIFO or other
exciting object (/dev/stdin, for example), it would falsely print
"file not found".  Instead: stat the object rather than using -f so
that we do not mind if the object is not a file; and print the errno
value in the error message.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
v6: New patch in this version of the series
---
 scripts/get_maintainer.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 07369aa..43fb5f5 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -381,8 +381,8 @@ foreach my $file (@ARGV) {
 	##if $file is a directory and it lacks a trailing slash, add one
 	if ((-d $file)) {
 	    $file =~ s@([^/])$@$1/@;
-	} elsif (!(-f $file)) {
-	    die "$P: file '${file}' not found\n";
+	} elsif (!(stat $file)) {
+	    die "$P: file '${file}' not found: $!\n";
 	}
     }
     if ($from_filename) {
-- 
2.1.4

Re: [Qemu-devel] [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory"):
> Anthony PERARD writes ("Re: [PATCH 10/12] xen: Use newly added dmops for mapping VGA memory"):
> > This patch seems to remove the last users of
> > xen_xc_domain_add_to_physmap(). Can it be remove from xen_common.h?
> > 
> > With that:
> > Acked-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Have added a separate patch for that.

Wrong patch.  Gah!

>From 8794b90b82612665d1e3f9f96ab73579a271c3be Mon Sep 17 00:00:00 2001
From: Ian Jackson <ian.jackson@eu.citrix.com>
Date: Fri, 9 Mar 2018 16:08:55 +0000
Subject: [PATCH 3/5] xen: Remove now-obsolete xen_xc_domain_add_to_physmap

The last user was just removed; remove this function, accordingly.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/hw/xen/xen_common.h | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
index 2eed6fc..5f1402b 100644
--- a/include/hw/xen/xen_common.h
+++ b/include/hw/xen/xen_common.h
@@ -645,28 +645,6 @@ static inline int xen_set_ioreq_server_state(domid_t dom,
 
 #endif
 
-#if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40600
-static inline int xen_xc_domain_add_to_physmap(xc_interface *xch, uint32_t domid,
-                                               unsigned int space,
-                                               unsigned long idx,
-                                               xen_pfn_t gpfn)
-{
-    return xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
-}
-#else
-static inline int xen_xc_domain_add_to_physmap(xc_interface *xch, uint32_t domid,
-                                               unsigned int space,
-                                               unsigned long idx,
-                                               xen_pfn_t gpfn)
-{
-    /* In Xen 4.6 rc is -1 and errno contains the error value. */
-    int rc = xc_domain_add_to_physmap(xch, domid, space, idx, gpfn);
-    if (rc == -1)
-        return errno;
-    return rc;
-}
-#endif
-
 #ifdef CONFIG_XEN_PV_DOMAIN_BUILD
 #if CONFIG_XEN_CTRL_INTERFACE_VERSION < 40700
 static inline int xen_domain_create(xc_interface *xc, uint32_t ssidref,
-- 
2.1.4

[Qemu-devel] [PATCH 11/12] xen: Expect xenstore write to fail when restricted

[Ian Jackson]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

Saving the current state to xenstore may fail when running restricted
(in particular, after a migration). Therefore, don't report the error or
exit when running restricted.  Toolstacks that want to allow running
QEMU restricted should instead make use of QMP events to listen for
state changes.

CC: Ian Jackson <ian.jackson@eu.citrix.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

---
v6: New patch in this version of the series
---
 hw/xen/xen-common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/xen/xen-common.c b/hw/xen/xen-common.c
index f73b416..8ede246 100644
--- a/hw/xen/xen-common.c
+++ b/hw/xen/xen-common.c
@@ -101,7 +101,12 @@ static void xenstore_record_dm_state(struct xs_handle *xs, const char *state)
     }
 
     snprintf(path, sizeof (path), "device-model/%u/state", xen_domid);
-    if (!xs_write(xs, XBT_NULL, path, state, strlen(state))) {
+    /*
+     * This call may fail when running restricted so don't make it fatal in
+     * that case. Toolstacks should instead use QMP to listen for state changes.
+     */
+    if (!xs_write(xs, XBT_NULL, path, state, strlen(state)) &&
+            !xen_domid_restrict) {
         error_report("error recording dm state");
         exit(1);
     }
-- 
2.1.4

Re: [Qemu-devel] [PATCH 11/12] xen: Expect xenstore write to fail when restricted

[Anthony PERARD]

On Thu, Mar 08, 2018 at 07:03:06PM +0000, Ian Jackson wrote:
> From: Ross Lagerwall <ross.lagerwall@citrix.com>
> 
> Saving the current state to xenstore may fail when running restricted
> (in particular, after a migration). Therefore, don't report the error or
> exit when running restricted.  Toolstacks that want to allow running
> QEMU restricted should instead make use of QMP events to listen for
> state changes.
> 
> CC: Ian Jackson <ian.jackson@eu.citrix.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> Reviewed-by: Ian Jackson <ian.jackson@eu.citrix.com>

Acked-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,

-- 
Anthony PERARD

[Qemu-devel] [PATCH 12/12] scripts/get_maintainer.pl: Print proper error message for missing $file

[Ian Jackson]

If you pass scripts/get_maintainer.pl the name of a FIFO or other
exciting object (/dev/stdin, for example), it would falsely print
"file not found".  Instead: stat the object rather than using -f so
that we do not mind if the object is not a file; and print the errno
value in the error message.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Thomas Huth <thuth@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
---
v6: New patch in this version of the series
---
 scripts/get_maintainer.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
index 07369aa..43fb5f5 100755
--- a/scripts/get_maintainer.pl
+++ b/scripts/get_maintainer.pl
@@ -381,8 +381,8 @@ foreach my $file (@ARGV) {
 	##if $file is a directory and it lacks a trailing slash, add one
 	if ((-d $file)) {
 	    $file =~ s@([^/])$@$1/@;
-	} elsif (!(-f $file)) {
-	    die "$P: file '${file}' not found\n";
+	} elsif (!(stat $file)) {
+	    die "$P: file '${file}' not found: $!\n";
 	}
     }
     if ($from_filename) {
-- 
2.1.4

Re: [Qemu-devel] [PATCH 12/12] scripts/get_maintainer.pl: Print proper error message for missing $file

[Paolo Bonzini]

On 08/03/2018 20:03, Ian Jackson wrote:
> If you pass scripts/get_maintainer.pl the name of a FIFO or other
> exciting object (/dev/stdin, for example), it would falsely print
> "file not found".  Instead: stat the object rather than using -f so
> that we do not mind if the object is not a file; and print the errno
> value in the error message.
> 
> Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Thomas Huth <thuth@redhat.com>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Anthony PERARD <anthony.perard@citrix.com>
> ---
> v6: New patch in this version of the series
> ---
>  scripts/get_maintainer.pl | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index 07369aa..43fb5f5 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -381,8 +381,8 @@ foreach my $file (@ARGV) {
>  	##if $file is a directory and it lacks a trailing slash, add one
>  	if ((-d $file)) {
>  	    $file =~ s@([^/])$@$1/@;
> -	} elsif (!(-f $file)) {
> -	    die "$P: file '${file}' not found\n";
> +	} elsif (!(stat $file)) {
> +	    die "$P: file '${file}' not found: $!\n";
>  	}
>      }
>      if ($from_filename) {
> 

Queued, thanks.

Paolo

Re: [Qemu-devel] [PATCH v6.1 00/11] xen: xen-domid-restrict improvements

[Ian Jackson]

I have folded in the comments so far and made a prototype v7 series,
which is here:

  http://xenbits.xen.org/gitweb/?p=people/iwj/qemu.git;a=summary
  https://xenbits.xen.org/git-http/people/iwj/qemu.git
  git://xenbits.xen.org/people/iwj/qemu.git

In the branch

  master..xen-restrict-v7.0

I have NOT been able to test it, unfortunately, as my test setup for
this had rotted and I am now going away for two weeks.  It does
compile.

I'm posting this here just because it seems bad form to go away with
the branch in a "secret" location on my workstation.  If someone else
wants to pick it up and shepherd it into the 2.12 release then that
would of course be fine by me; if not I will pick it up when I get
back.

I have some more work to build on top of this anyway, including wiring
this into the Xen CI, before it can be declared properly supported by
Xen.

In the meantime maybe the fixes to get_maintainer and checkpatch are
independent of the rest and should probably be applied as soon as
practical (subject to any further review that may be needed).  that
is:

 [PATCH 01/12] checkpatch: Add xendevicemodel_handle to the list of types
 [PATCH 12/12] scripts/get_maintainer.pl: Print proper error message for missing $file

Or from my branch above,

 cb3ebff4087a checkpatch: Add xendevicemodel_handle to the list of types
 15dedc627cad scripts/get_maintainer.pl: Print proper error message for missing $file

Regards,
Ian.