Re: [Xen-devel] [PATCH v2 03/13] xen/xsplice: Hypervisor implementation of XEN_XSPLICE_op (v7)

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

> > +# Enable/Disable xsplice support
> > +config XSPLICE
> > +   bool "xsplice support"
> > +   default y
> > +   depends on HAS_XSPLICE
> > +   ---help---
> > +     Allows a running Xen hypervisor to be patched without rebooting.
> > +     This is primarily used to patch an hypervisor with XSA fixes.
> > +
> > +     If unsure, say Y.
> > +
> >  endmenu
> 
> I'm indifferent on the HAS_XSPLICE, you can drop that if you want to
> simply stuff.

OK, let me remove it. Thanks!
.. snip..
> > +#ifdef CONFIG_XSPLICE
> > +    case XEN_SYSCTL_xsplice_op:
> > +        ret = xsplice_control(&op->u.xsplice);
> > +        copyback = 1;
> > +        break;
> > +#endif
> 
> Should the case statement still exist and not just return -ENOSYS?
> Otherwise we're needlessly going into arch_do_sysctl() just to get the
> same result.

There is some other code in that function, such as:

398 
399 #ifdef TEST_COVERAGE
400     case XEN_SYSCTL_coverage_op:
401         ret = sysctl_coverage_op(&op->u.coverage_op);
402         break;
403 #endif


Which follows that pattern. I figured I would the same thing as the rest
of the code around it. It is not that neat ..

But I could also make this a bit neater and have the header file return
-ENOSYS, like this: 

diff --git a/xen/common/sysctl.c b/xen/common/sysctl.c
index b81b8cd..a8355c5 100644
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -461,12 +461,11 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
u_sysctl)
         ret = tmem_control(&op->u.tmem_op);
         break;
 
-#ifdef CONFIG_XSPLICE
     case XEN_SYSCTL_xsplice_op:
         ret = xsplice_control(&op->u.xsplice);
-        copyback = 1;
+        if ( ret != -ENOSYS )
+            copyback = 1;
         break;
-#endif
 
     default:
         ret = arch_do_sysctl(op, u_sysctl);
diff --git a/xen/include/xen/xsplice.h b/xen/include/xen/xsplice.h
index 3c1b2e4..a0ae95e 100644
--- a/xen/include/xen/xsplice.h
+++ b/xen/include/xen/xsplice.h
@@ -23,15 +23,18 @@ struct xsplice_patch_func {
     uint8_t pad[24];
 };
 
-int xsplice_control(struct xen_sysctl_xsplice_op *);
-
 #ifdef CONFIG_XSPLICE
+int xsplice_control(struct xen_sysctl_xsplice_op *);
 void do_xsplice(void);
 struct bug_frame *xsplice_find_bug(const char *eip, int *id);
 bool_t is_module(const void *addr);
 bool_t is_active_module_text(unsigned long addr);
 unsigned long search_module_extables(unsigned long addr);
 #else
+static inline xsplice_control(struct xen_sysctl_xsplice_op *)
+{
+    return -ENOSYS;
+}
 static inline void do_xsplice(void) { };
 static inline struct bug_frame *xsplice_find_bug(const char *eip, int *id)
 {

Yeah, let me roll that in.
Thanks!

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