Samuel Thibault
2008-Mar-29 11:23 UTC
[Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Mar-29 11:47 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Mar-29 11:58 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Apr-05 14:28 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Apr-05 16:25 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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); _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2008-Apr-05 16:31 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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)_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Apr-10 15:43 UTC
[Xen-devel] ioemu & settimeoffset [Was: Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain()]
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() _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Apr-11 14:27 UTC
Re: [Xen-devel] Re: [Xen-changelog] [xen-unstable] Clean up handling of IS_PRIV_FOR() and rcu_[un]lock_domain().
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__ */ _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel