Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Samuel Thibault]

Hello,

Xen patchbot-unstable, le Sat 29 Mar 2008 00:50:07 -0700, a écrit :
> This probably breaks stub domains. Where necessary, some of these
> reversions can themselves be reverted where they are judged both
> necessary and safe.

HVMOP_get_param is needed yes (set_param doesn't seem to be).

set_foreigndom is probably needed in a lot of cases, but maybe not all,
so maybe we should have two versions of it.

DOMCTL_getdomaininfo is needed.
DOMCTL_max_mem is needed.
DOMCTL_settimeoffset is needed.
x86 DOMCTL_memory/ioport_mapping are needed for passthrough (not
implemented yet, though)

IIRC the event channel ops are not needed right now, but will probably
be in the future.

XENMEM_in/decrease_reservation and populate_physmap are needed.
XENMEM_maximum_gpfn is needed.

I don't have the time to test precisely what else would be needed, but
the cases above should be at least 90% of what is.

Samuel

Re: Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Keir Fraser]

On 29/3/08 11:23, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:

> HVMOP_get_param is needed yes (set_param doesn't seem to be).

I didn't revert any of the HVMOP ones. Basically, anything that a domain is
allowed to do to itself I also kept the IS_PRIV_FOR() case.

> set_foreigndom is probably needed in a lot of cases, but maybe not all,
> so maybe we should have two versions of it.

We can keep it as is. Really TLB flush masks should have the IS_PRIV_FOR()
domain ORed in though.

> DOMCTL_getdomaininfo is needed.
> DOMCTL_max_mem is needed.

These ones are a sticking point I'm afraid. DOMCTL_max_mem is a globally
privileged operation because it can give increased access to the global
memory resource. We can't let stub domains have at it. We'll have to keep
max_mem permanently increased, and set that up in xend. With that done you
probably don't really need getdomaininfo either.

> DOMCTL_settimeoffset is needed.

Why is this done in ioemu and not in xend (it's already done there for PV
guests).

> x86 DOMCTL_memory/ioport_mapping are needed for passthrough (not
> implemented yet, though)

Again, they are globally privileged operations. I can't see that they
logically belong in ioemu-dm in the first place. It's not necessarily your
job to clean this up of course, but it simply means that passthru is
incompatible with stub domains until it is cleaned up.

> IIRC the event channel ops are not needed right now, but will probably
> be in the future.

They were all fine, except there was one inexplicable check of IS_PRIV_FOR()
in bind_interdomain() which I nuked. It was so bizarre that I assumed you
must have put it there for a reason, and this would be one that you'd
complain about. If not, then great! :-)

> XENMEM_in/decrease_reservation and populate_physmap are needed.
> XENMEM_maximum_gpfn is needed.

All these I allowed, by the reasoning that DOMID_SELF is allowed them and
that should allow IS_PRIV_FOR() too.

> I don't have the time to test precisely what else would be needed, but
> the cases above should be at least 90% of what is.

Sounds to me like the domctls are the sticking point. And I can't reasonably
budge on those.

 -- Keir

Re: Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Samuel Thibault]

Keir Fraser, le Sat 29 Mar 2008 11:47:57 +0000, a écrit :
> > DOMCTL_settimeoffset is needed.
> 
> Why is this done in ioemu and not in xend (it's already done there for PV
> guests).

I haven't looked the details, but basically that's done at
initialization, reading from xenstore's rtc/timeoffset.

> > x86 DOMCTL_memory/ioport_mapping are needed for passthrough (not
> > implemented yet, though)
> 
> it simply means that passthru is incompatible with stub domains until
> it is cleaned up.

Ok.

I'll check the remaining details when I get back.

Samuel

Re: Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Samuel Thibault]

Hello,

Keir Fraser, le Sat 29 Mar 2008 11:47:57 +0000, a écrit :
> > IIRC the event channel ops are not needed right now, but will probably
> > be in the future.
> 
> They were all fine, except there was one inexplicable check of IS_PRIV_FOR()
> in bind_interdomain() which I nuked. It was so bizarre that I assumed you
> must have put it there for a reason, and this would be one that you'd
> complain about.

I'm now complaining :)

The bind_interdomain() trick is needed for the ioreq events channel:
when it gets installed, it is supposed to be between the HVM domain and
dom0 (the stub domain doesn't exist anyway).  The meaning of the test is
hence to allow the stub domain to hijack that event channel (because it
has privileges on the remote domain).

With that fix, stub domains are working again (but Cirrus bios doesn't
work yet)

Samuel

Re: Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Keir Fraser]

On 5/4/08 15:28, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:

>> They were all fine, except there was one inexplicable check of IS_PRIV_FOR()
>> in bind_interdomain() which I nuked. It was so bizarre that I assumed you
>> must have put it there for a reason, and this would be one that you'd
>> complain about.
> 
> I'm now complaining :)
> 
> The bind_interdomain() trick is needed for the ioreq events channel:
> when it gets installed, it is supposed to be between the HVM domain and
> dom0 (the stub domain doesn't exist anyway).  The meaning of the test is
> hence to allow the stub domain to hijack that event channel (because it
> has privileges on the remote domain).

The hack kind of sucks. :-) Add a new hvm_param to indicate the device model
domain. Default it to zero, and if it becomes set to some other value (by
the stub domain itself, when it starts) then re-create the event-channel
port with new remote domid.

 -- Keir

> With that fix, stub domains are working again (but Cirrus bios doesn't
> work yet)

Re: Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Samuel Thibault]

Keir Fraser, le Sat 05 Apr 2008 17:31:39 +0100, a écrit :
> On 5/4/08 15:28, "Samuel Thibault" <samuel.thibault@eu.citrix.com> wrote:
> 
> >> They were all fine, except there was one inexplicable check of IS_PRIV_FOR()
> >> in bind_interdomain() which I nuked. It was so bizarre that I assumed you
> >> must have put it there for a reason, and this would be one that you'd
> >> complain about.
> > 
> > I'm now complaining :)
> > 
> > The bind_interdomain() trick is needed for the ioreq events channel:
> > when it gets installed, it is supposed to be between the HVM domain and
> > dom0 (the stub domain doesn't exist anyway).  The meaning of the test is
> > hence to allow the stub domain to hijack that event channel (because it
> > has privileges on the remote domain).
> 
> The hack kind of sucks. :-) Add a new hvm_param to indicate the device model
> domain. Default it to zero, and if it becomes set to some other value (by
> the stub domain itself, when it starts) then re-create the event-channel
> port with new remote domid.



hvm: Add HVM_PARAM_DM_DOMAIN to let ioreq events go to a stub domain
instead of dom0.

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r fec296bcfd21 tools/ioemu/hw/xen_machine_fv.c
--- a/tools/ioemu/hw/xen_machine_fv.c	Fri Apr 11 09:57:06 2008 +0100
+++ b/tools/ioemu/hw/xen_machine_fv.c	Fri Apr 11 15:27:36 2008 +0100
@@ -205,6 +205,7 @@ static void xen_init_fv(uint64_t ram_siz
     }
 #endif
 
+    xc_set_hvm_param(xc_handle, domid, HVM_PARAM_DM_DOMAIN, DOMID_SELF);
     xc_get_hvm_param(xc_handle, domid, HVM_PARAM_IOREQ_PFN, &ioreq_pfn);
     fprintf(logfile, "shared page at pfn %lx\n", ioreq_pfn);
     shared_page = xc_map_foreign_range(xc_handle, domid, XC_PAGE_SIZE,
diff -r fec296bcfd21 xen/arch/ia64/vmx/vmx_hypercall.c
--- a/xen/arch/ia64/vmx/vmx_hypercall.c	Fri Apr 11 09:57:06 2008 +0100
+++ b/xen/arch/ia64/vmx/vmx_hypercall.c	Fri Apr 11 15:27:36 2008 +0100
@@ -165,6 +165,23 @@ do_hvm_op(unsigned long op, XEN_GUEST_HA
                 iorp = &d->arch.hvm_domain.buf_pioreq;
                 rc = vmx_set_ioreq_page(d, iorp, a.value);
                 break;
+            case HVM_PARAM_DM_DOMAIN:
+                /* Recreate ioreq event channels */
+                if (a.value == DOMID_SELF)
+                    a.value = current->domain->domain_id;
+                iorp = &d->arch.hvm_domain.ioreq;
+                for_each_vcpu ( d, v ) {
+                    rc = alloc_unbound_xen_event_channel(v, a.value);
+                    if (rc < 0)
+                        goto param_fail;
+                    free_xen_event_channel(v, v->arch.arch_vmx.xen_port);
+                    v->arch.arch_vmx.xen_port = rc;
+                    spin_lock(&iorp->lock);
+                    if (iorp->va != NULL)
+                        get_ioreq(v)->vp_eport = rc;
+                    spin_unlock(&iorp->lock);
+                }
+                break;
             default:
                 /* nothing */
                 break;
diff -r fec296bcfd21 xen/arch/x86/hvm/hvm.c
--- a/xen/arch/x86/hvm/hvm.c	Fri Apr 11 09:57:06 2008 +0100
+++ b/xen/arch/x86/hvm/hvm.c	Fri Apr 11 15:27:36 2008 +0100
@@ -2239,6 +2239,23 @@ long do_hvm_op(unsigned long op, XEN_GUE
 
                 domain_unpause(d);
                 break;
+            case HVM_PARAM_DM_DOMAIN:
+                /* Recreate ioreq event channels */
+                if (a.value == DOMID_SELF)
+                    a.value = current->domain->domain_id;
+                iorp = &d->arch.hvm_domain.ioreq;
+                for_each_vcpu ( d, v ) {
+                    rc = alloc_unbound_xen_event_channel(v, a.value);
+                    if (rc < 0)
+                        goto param_fail;
+                    free_xen_event_channel(v, v->arch.hvm_vcpu.xen_port);
+                    v->arch.hvm_vcpu.xen_port = rc;
+                    spin_lock(&iorp->lock);
+                    if (iorp->va != NULL)
+                        get_ioreq(v)->vp_eport = rc;
+                    spin_unlock(&iorp->lock);
+                }
+                break;
             }
             d->arch.hvm_domain.params[a.index] = a.value;
             rc = 0;
diff -r fec296bcfd21 xen/include/public/hvm/params.h
--- a/xen/include/public/hvm/params.h	Fri Apr 11 09:57:06 2008 +0100
+++ b/xen/include/public/hvm/params.h	Fri Apr 11 15:27:36 2008 +0100
@@ -85,6 +85,9 @@
 #define HVM_PARAM_HPET_ENABLED 11
 #define HVM_PARAM_IDENT_PT     12
 
-#define HVM_NR_PARAMS          13
+/* Device Model domain, defaults to 0 */
+#define HVM_PARAM_DM_DOMAIN    13
+
+#define HVM_NR_PARAMS          14
 
 #endif /* __XEN_PUBLIC_HVM_PARAMS_H__ */

Re: Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().

[Samuel Thibault]

Keir Fraser, le Sat 29 Mar 2008 11:47:57 +0000, a écrit :
> > DOMCTL_getdomaininfo is needed.
> > DOMCTL_max_mem is needed.
> 
> These ones are a sticking point I'm afraid. DOMCTL_max_mem is a globally
> privileged operation because it can give increased access to the global
> memory resource. We can't let stub domains have at it. We'll have to keep
> max_mem permanently increased, and set that up in xend. With that done you
> probably don't really need getdomaininfo either.

Hum, actually that was already done, see python/xen/xend/Image.py:682
and that can indeed be seen in xm top...
The patch below drops the qemu reservation code, and things still work
as expected.

Samuel

ioemu: drop duplicate memory reservation

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 30c502e58777 tools/ioemu/vl.c
--- a/tools/ioemu/vl.c	Sat Apr 05 15:01:14 2008 +0100
+++ b/tools/ioemu/vl.c	Sat Apr 05 17:18:56 2008 +0100
@@ -7018,26 +7018,12 @@ int unset_mm_mapping(int xc_handle, uint
                      xen_pfn_t *extent_start)
 {
     int err = 0;
-    xc_dominfo_t info;
-
-    xc_domain_getinfo(xc_handle, domid, 1, &info);
-    if ((info.nr_pages - nr_pages) <= 0) {
-        fprintf(stderr, "unset_mm_mapping: error nr_pages\n");
-        err = -1;
-    }
 
     err = xc_domain_memory_decrease_reservation(xc_handle, domid,
                                                 nr_pages, 0, extent_start);
     if (err)
         fprintf(stderr, "Failed to decrease physmap\n");
 
-
-    if (xc_domain_setmaxmem(xc_handle, domid, (info.nr_pages - nr_pages) *
-                            PAGE_SIZE/1024) != 0) {
-        fprintf(logfile, "set maxmem returned error %d\n", errno);
-        err = -1;
-    }
-
     return err;
 }
 
@@ -7045,16 +7031,7 @@ int set_mm_mapping(int xc_handle, uint32
                    unsigned long nr_pages, unsigned int address_bits,
                    xen_pfn_t *extent_start)
 {
-    xc_dominfo_t info;
     int err = 0;
-
-    xc_domain_getinfo(xc_handle, domid, 1, &info);
-
-    if (xc_domain_setmaxmem(xc_handle, domid, info.max_memkb +
-                            nr_pages * PAGE_SIZE/1024) != 0) {
-        fprintf(logfile, "set maxmem returned error %d\n", errno);
-        return -1;
-    }
 
     err = xc_domain_memory_populate_physmap(xc_handle, domid, nr_pages, 0,
                                             address_bits, extent_start);

ioemu & settimeoffset [Was: Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain()]

[Samuel Thibault]

Keir Fraser, le Sat 29 Mar 2008 11:47:57 +0000, a écrit :
> > DOMCTL_settimeoffset is needed.
> 
> Why is this done in ioemu and not in xend (it's already done there for PV
> guests).

I don't see a reason indeed, the attached patch seems to work fine.

Samuel


Make xend set time offset for all kinds of domains,
so that ioemu doesn't need to do it.

Signed-off-by: Samuel Thibault <samuel.thibault@eu.citrix.com>

diff -r 8d750b7acfa3 tools/ioemu/target-i386-dm/helper2.c
--- a/tools/ioemu/target-i386-dm/helper2.c	Thu Apr 10 11:11:25 2008 +0100
+++ b/tools/ioemu/target-i386-dm/helper2.c	Thu Apr 10 16:36:05 2008 +0100
@@ -391,8 +391,6 @@
 	fprintf(logfile, "Time offset set %ld\n", time_offset);
     else
 	time_offset = 0;
-
-    xc_domain_set_time_offset(xc_handle, domid, time_offset);
 
     free(p);
 }
--- a/tools/python/xen/xend/image.py	Thu Apr 10 11:11:25 2008 +0100
+++ b/tools/python/xen/xend/image.py	Thu Apr 10 16:36:05 2008 +0100
@@ -99,7 +99,9 @@ ImageHandler configure
         self.vncconsole = vmConfig['platform'].get('vncconsole')
         self.dmargs = self.parseDeviceModelArgs(vmConfig)
         self.pid = None
-
+        rtc_timeoffset = vmConfig['platform'].get('rtc_timeoffset')
+        if rtc_timeoffset is not None:
+            xc.domain_set_time_offset(self.vm.getDomid(), int(rtc_timeoffset))
 
 
     def cleanupBootloading(self):
@@ -419,9 +421,6 @@ LinuxImageHandler configure
 
     def configure(self, vmConfig):
         ImageHandler.configure(self, vmConfig)
-        rtc_timeoffset = vmConfig['platform'].get('rtc_timeoffset')
-        if rtc_timeoffset is not None:
-            xc.domain_set_time_offset(self.vm.getDomid(), int(rtc_timeoffset))
 
     def buildDomain(self):
         store_evtchn = self.vm.getStorePort()