| Message ID | 20240830175309.2854442-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tools/ocaml/xc: Drop the GC lock for all hypercalls | expand |
> On 30 Aug 2024, at 18:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > We should be doing this unilaterally. Acked-by: Christian Lindig <christian.lindig@cloud.com> I would prefer to use caml_release_runtime_system(), caml_acquire_runtime_system() which I think is a better name for these functions. This could be likewise changed globally. https://ocaml.org/manual/5.2/intfc.html#ss:parallel-execution-long-running-c-code — C
[gmail is a bit terrible and defaults to reply to single person not reply all, resent] There is one bug here that would cause a crash, and several instances of undefined behaviour. On Mon, Sep 2, 2024 at 9:10 AM Edwin Torok <edwin.torok@cloud.com> wrote: > > On Fri, Aug 30, 2024 at 6:53 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > > > We should be doing this unilaterally. > > Agreed, but we should do it safely, since last time I did this I > learned about a few more instances of behaviours I previously thought > to be safe, but that are undefined behaviour. > Which probably means we have a bunch of other code to fixup (I should > really finish my static analyzer project, and update it with the newly > learned rules to catch all these...). > See below for comments. > > Although there is one bug here we've previously known to avoid: > String_val cannot be dereferenced with the lock released, that one is > an OCaml value and will cause actual problems, > so we need to caml_copy_string that one. > > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > --- > > CC: Christian Lindig <christian.lindig@citrix.com> > > CC: David Scott <dave@recoil.org> > > CC: Edwin Török <edwin.torok@cloud.com> > > CC: Rob Hoes <Rob.Hoes@citrix.com> > > CC: Andrii Sultanov <andrii.sultanov@cloud.com> > > > > Also pulled out of a larger cleanup series. > > --- > > tools/ocaml/libs/xc/xenctrl_stubs.c | 63 +++++++++++++++++++++++++++-- > > 1 file changed, 60 insertions(+), 3 deletions(-) > > > > diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c > > index c78191f95abc..20487b21008f 100644 > > --- a/tools/ocaml/libs/xc/xenctrl_stubs.c > > +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c > > @@ -312,7 +312,10 @@ CAMLprim value stub_xc_domain_max_vcpus(value xch_val, value domid, > > xc_interface *xch = xch_of_val(xch_val); > > int r; > > > > + caml_enter_blocking_section(); > > r = xc_domain_max_vcpus(xch, Int_val(domid), Int_val(max_vcpus)); > > We need to move the Int_val macros out of here, domid is registered as > a GC root, so the GC *will* write to it (it'll write the same value). > So in practice it probably won't cause any observable corruption, but > is still undefined behaviour and may not play nicely with compiler > optimizations. > > This would probably be easier to review in a git tree, because there > isn't enough context in the patch to see which values are registered > as GC roots or not. > > > + caml_leave_blocking_section(); > > + > > if (r) > > failwith_xc(xch); > > > > @@ -329,7 +332,10 @@ value stub_xc_domain_sethandle(value xch_val, value domid, value handle) > > > > domain_handle_of_uuid_string(h, String_val(handle)); > > > > + caml_enter_blocking_section(); > > i = xc_domain_sethandle(xch, Int_val(domid), h); > > GC root, need to move the macro out and assign to a local value. > > > + caml_leave_blocking_section(); > > + > > if (i) > > failwith_xc(xch); > > > > @@ -391,7 +397,10 @@ CAMLprim value stub_xc_domain_shutdown(value xch_val, value domid, value reason) > > xc_interface *xch = xch_of_val(xch_val); > > int ret; > > > > + caml_enter_blocking_section(); > > ret = xc_domain_shutdown(xch, Int_val(domid), Int_val(reason)); > > GC roots again. > > > + caml_leave_blocking_section(); > > + > > if (ret < 0) > > failwith_xc(xch); > > > > @@ -503,7 +512,10 @@ CAMLprim value stub_xc_domain_getinfo(value xch_val, value domid) > > xc_domaininfo_t info; > > int ret; > > > > + caml_enter_blocking_section(); > > ret = xc_domain_getinfo_single(xch, Int_val(domid), &info); > > GC root. > > > + caml_leave_blocking_section(); > > + > > if (ret < 0) > > failwith_xc(xch); > > > > @@ -546,7 +558,10 @@ CAMLprim value stub_xc_vcpu_context_get(value xch_val, value domid, > > int ret; > > vcpu_guest_context_any_t ctxt; > > > > + caml_enter_blocking_section(); > > ret = xc_vcpu_getcontext(xch, Int_val(domid), Int_val(cpu), &ctxt); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if ( ret < 0 ) > > failwith_xc(xch); > > > > @@ -584,10 +599,14 @@ CAMLprim value stub_xc_vcpu_setaffinity(value xch_val, value domid, > > if (Bool_val(Field(cpumap, i))) > > c_cpumap[i/8] |= 1 << (i&7); > > } > > + > > + caml_enter_blocking_section(); > > retval = xc_vcpu_setaffinity(xch, Int_val(domid), > > Int_val(vcpu), > > c_cpumap, NULL, > > XEN_VCPUAFFINITY_HARD); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > free(c_cpumap); > > > > if (retval < 0) > > @@ -612,10 +631,13 @@ CAMLprim value stub_xc_vcpu_getaffinity(value xch_val, value domid, > > if (c_cpumap == NULL) > > failwith_xc(xch); > > > > + caml_enter_blocking_section(); > > retval = xc_vcpu_getaffinity(xch, Int_val(domid), > > Int_val(vcpu), > > c_cpumap, NULL, > > XEN_VCPUAFFINITY_HARD); > > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (retval < 0) { > > free(c_cpumap); > > failwith_xc(xch); > > @@ -639,9 +661,13 @@ CAMLprim value stub_xc_sched_id(value xch_val) > > { > > CAMLparam1(xch_val); > > xc_interface *xch = xch_of_val(xch_val); > > - int sched_id; > > + int ret, sched_id; > > + > > + caml_enter_blocking_section(); > > + ret = xc_sched_id(xch, &sched_id); > > + caml_leave_blocking_section(); > > > > - if (xc_sched_id(xch, &sched_id)) > > + if (ret) > > failwith_xc(xch); > > > > CAMLreturn(Val_int(sched_id)); > > @@ -674,7 +700,10 @@ CAMLprim value stub_xc_evtchn_reset(value xch_val, value domid) > > xc_interface *xch = xch_of_val(xch_val); > > int r; > > > > + caml_enter_blocking_section(); > > r = xc_evtchn_reset(xch, Int_val(domid)); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (r < 0) > > failwith_xc(xch); > > CAMLreturn(Val_unit); > > @@ -811,7 +840,10 @@ CAMLprim value stub_xc_send_debug_keys(value xch_val, value keys) > > xc_interface *xch = xch_of_val(xch_val); > > int r; > > > > + caml_enter_blocking_section(); > > r = xc_send_debug_keys(xch, String_val(keys)); > > This is clearly unsafe because String_val dereferences an OCaml value > with the lock released, which is not allowed. > You need to copy the string to a C string, and free it afterwards. > > > + caml_leave_blocking_section(); > > + > > if (r) > > failwith_xc(xch); > > CAMLreturn(Val_unit); > > @@ -952,7 +984,11 @@ CAMLprim value stub_xc_domain_set_memmap_limit(value xch_val, value domid, > > int retval; > > > > v = Int64_val(map_limitkb); > > + > > + caml_enter_blocking_section(); > > retval = xc_domain_set_memmap_limit(xch, Int_val(domid), v); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (retval) > > failwith_xc(xch); > > > > @@ -1203,8 +1239,11 @@ CAMLprim value stub_xc_domain_ioport_permission(value xch_val, value domid, > > c_nr_ports = Int_val(nr_ports); > > c_allow = Bool_val(allow); > > > > + caml_enter_blocking_section(); > > ret = xc_domain_ioport_permission(xch, Int_val(domid), > > c_start_port, c_nr_ports, c_allow); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (ret < 0) > > failwith_xc(xch); > > > > @@ -1225,8 +1264,11 @@ CAMLprim value stub_xc_domain_iomem_permission(value xch_val, value domid, > > c_nr_pfns = Nativeint_val(nr_pfns); > > c_allow = Bool_val(allow); > > > > + caml_enter_blocking_section(); > > ret = xc_domain_iomem_permission(xch, Int_val(domid), > > c_start_pfn, c_nr_pfns, c_allow); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (ret < 0) > > failwith_xc(xch); > > > > @@ -1245,8 +1287,11 @@ CAMLprim value stub_xc_domain_irq_permission(value xch_val, value domid, > > c_pirq = Int_val(pirq); > > c_allow = Bool_val(allow); > > > > + caml_enter_blocking_section(); > > ret = xc_domain_irq_permission(xch, Int_val(domid), > > c_pirq, c_allow); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (ret < 0) > > failwith_xc(xch); > > > > @@ -1309,7 +1354,9 @@ CAMLprim value stub_xc_domain_test_assign_device(value xch_val, value domid, val > > func = Int_val(Field(desc, 3)); > > sbdf = encode_sbdf(domain, bus, dev, func); > > > > + caml_enter_blocking_section(); > > ret = xc_test_assign_device(xch, Int_val(domid), sbdf); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > > > CAMLreturn(Val_bool(ret == 0)); > > } > > @@ -1328,8 +1375,10 @@ CAMLprim value stub_xc_domain_assign_device(value xch_val, value domid, value de > > func = Int_val(Field(desc, 3)); > > sbdf = encode_sbdf(domain, bus, dev, func); > > > > + caml_enter_blocking_section(); > > ret = xc_assign_device(xch, Int_val(domid), sbdf, > > XEN_DOMCTL_DEV_RDM_RELAXED); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > > > if (ret < 0) > > failwith_xc(xch); > > @@ -1350,7 +1399,9 @@ CAMLprim value stub_xc_domain_deassign_device(value xch_val, value domid, value > > func = Int_val(Field(desc, 3)); > > sbdf = encode_sbdf(domain, bus, dev, func); > > > > + caml_enter_blocking_section(); > > ret = xc_deassign_device(xch, Int_val(domid), sbdf); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > > > if (ret < 0) > > failwith_xc(xch); > > @@ -1379,8 +1430,11 @@ CAMLprim value stub_xc_get_cpu_featureset(value xch_val, value idx) > > /* To/from hypervisor to retrieve actual featureset */ > > uint32_t fs[fs_len], len = fs_len; > > unsigned int i; > > + int ret; > > > > - int ret = xc_get_cpu_featureset(xch, Int_val(idx), &len, fs); > > + caml_enter_blocking_section(); > > + ret = xc_get_cpu_featureset(xch, Int_val(idx), &len, fs); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > > > if (ret) > > failwith_xc(xch); > > @@ -1403,7 +1457,10 @@ CAMLprim value stub_xc_watchdog(value xch_val, value domid, value timeout) > > int ret; > > unsigned int c_timeout = Int32_val(timeout); > > > > + caml_enter_blocking_section(); > > ret = xc_watchdog(xch, Int_val(domid), c_timeout); > > Haven't checked these anymore, but I assume they are GC roots. > > > + caml_leave_blocking_section(); > > + > > if (ret < 0) > > failwith_xc(xch); > > > > -- > > 2.39.2 > >
On 02/09/2024 9:12 am, Christian Lindig wrote: >> On 30 Aug 2024, at 18:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> We should be doing this unilaterally. > Acked-by: Christian Lindig <christian.lindig@cloud.com> Thanks. > > I would prefer to use caml_release_runtime_system(), caml_acquire_runtime_system() which I think is a better name for these functions. This could be likewise changed globally. > > https://ocaml.org/manual/5.2/intfc.html#ss:parallel-execution-long-running-c-code This is a mess. Despite existing for 14 years, Ocaml (threads.h) still does this: CAMLextern void caml_enter_blocking_section (void); CAMLextern void caml_leave_blocking_section (void); #define caml_acquire_runtime_system caml_leave_blocking_section #define caml_release_runtime_system caml_enter_blocking_section So the "new" names are implemented in terms of the "old" ones, not the other way around. Do you mind if we defer the rename until a later point? For better or worse, Xen uses the old names consistently. ~Andrew
On 02/09/2024 9:10 am, Edwin Torok wrote: > On Fri, Aug 30, 2024 at 6:53 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> We should be doing this unilaterally. > Agreed, but we should do it safely, since last time I did this I > learned about a few more instances of behaviours I previously thought > to be safe, but that are undefined behaviour. > Which probably means we have a bunch of other code to fixup (I should > really finish my static analyzer project, and update it with the newly > learned rules to catch all these...). > See below for comments. > > Although there is one bug here we've previously known to avoid: > String_val cannot be dereferenced with the lock released, that one is > an OCaml value and will cause actual problems, > so we need to caml_copy_string that one. > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Christian Lindig <christian.lindig@citrix.com> >> CC: David Scott <dave@recoil.org> >> CC: Edwin Török <edwin.torok@cloud.com> >> CC: Rob Hoes <Rob.Hoes@citrix.com> >> CC: Andrii Sultanov <andrii.sultanov@cloud.com> >> >> Also pulled out of a larger cleanup series. >> --- >> tools/ocaml/libs/xc/xenctrl_stubs.c | 63 +++++++++++++++++++++++++++-- >> 1 file changed, 60 insertions(+), 3 deletions(-) >> >> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c >> index c78191f95abc..20487b21008f 100644 >> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c >> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c >> @@ -312,7 +312,10 @@ CAMLprim value stub_xc_domain_max_vcpus(value xch_val, value domid, >> xc_interface *xch = xch_of_val(xch_val); >> int r; >> >> + caml_enter_blocking_section(); >> r = xc_domain_max_vcpus(xch, Int_val(domid), Int_val(max_vcpus)); > We need to move the Int_val macros out of here, domid is registered as > a GC root, so the GC *will* write to it (it'll write the same value). How? As value's, both domid and max_vcpu are integers living in GPRs. These expressions are not dereferencing into the Ocaml Heap. ~Andrew
On Mon, Sep 2, 2024 at 5:38 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 02/09/2024 9:10 am, Edwin Torok wrote: > > On Fri, Aug 30, 2024 at 6:53 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote: > >> We should be doing this unilaterally. > > Agreed, but we should do it safely, since last time I did this I > > learned about a few more instances of behaviours I previously thought > > to be safe, but that are undefined behaviour. > > Which probably means we have a bunch of other code to fixup (I should > > really finish my static analyzer project, and update it with the newly > > learned rules to catch all these...). > > See below for comments. > > > > Although there is one bug here we've previously known to avoid: > > String_val cannot be dereferenced with the lock released, that one is > > an OCaml value and will cause actual problems, > > so we need to caml_copy_string that one. > > > >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > >> --- > >> CC: Christian Lindig <christian.lindig@citrix.com> > >> CC: David Scott <dave@recoil.org> > >> CC: Edwin Török <edwin.torok@cloud.com> > >> CC: Rob Hoes <Rob.Hoes@citrix.com> > >> CC: Andrii Sultanov <andrii.sultanov@cloud.com> > >> > >> Also pulled out of a larger cleanup series. > >> --- > >> tools/ocaml/libs/xc/xenctrl_stubs.c | 63 +++++++++++++++++++++++++++-- > >> 1 file changed, 60 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c > >> index c78191f95abc..20487b21008f 100644 > >> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c > >> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c > >> @@ -312,7 +312,10 @@ CAMLprim value stub_xc_domain_max_vcpus(value xch_val, value domid, > >> xc_interface *xch = xch_of_val(xch_val); > >> int r; > >> > >> + caml_enter_blocking_section(); > >> r = xc_domain_max_vcpus(xch, Int_val(domid), Int_val(max_vcpus)); > > We need to move the Int_val macros out of here, domid is registered as > > a GC root, so the GC *will* write to it (it'll write the same value). > > How? > > As value's, both domid and max_vcpu are integers living in GPRs. domid and max_vcpus are registered with CAMLparam macro, so their address is registered as a GC root, which the GC will see and go and update. The compiler may have cached this in a GPR, but we can't rely on that. In particular if you run the thread sanitizer in OCaml 5 it'll warn all over the place about this (I haven't gotten as far as having a full build with Xen and OCaml 5, I only have a XAPI build so far). See here for a discussion https://github.com/ocaml/ocaml/pull/13188. The GC could avoid the potential race here by checking whether the value is an integer prior to updating it, but that might have a (small) performance impact, so it is not done. > > These expressions are not dereferencing into the Ocaml Heap. No, but they are reachable from a C pointer that is registered as a GC root in the OCaml GC. (the CAMLparam macro registers all its parameters as GC roots). Another option is to not register integers with CAMLparam, but that breaks symmetry, and it'd then be very easy to forget to register an OCaml parameter. Best regards, --Edwin > > ~Andrew
> On 2 Sep 2024, at 17:13, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 02/09/2024 9:12 am, Christian Lindig wrote: >>> On 30 Aug 2024, at 18:53, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >>> >>> We should be doing this unilaterally. >> Acked-by: Christian Lindig <christian.lindig@cloud.com> > > Thanks. > >> >> I would prefer to use caml_release_runtime_system(), caml_acquire_runtime_system() which I think is a better name for these functions. This could be likewise changed globally. >> >> https://ocaml.org/manual/5.2/intfc.html#ss:parallel-execution-long-running-c-code > > This is a mess. > > Despite existing for 14 years, Ocaml (threads.h) still does this: > > CAMLextern void caml_enter_blocking_section (void); > CAMLextern void caml_leave_blocking_section (void); > #define caml_acquire_runtime_system caml_leave_blocking_section > #define caml_release_runtime_system caml_enter_blocking_section > > So the "new" names are implemented in terms of the "old" ones, not the > other way around. > > Do you mind if we defer the rename until a later point? For better or > worse, Xen uses the old names consistently. That’s fine. — C
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c index c78191f95abc..20487b21008f 100644 --- a/tools/ocaml/libs/xc/xenctrl_stubs.c +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c @@ -312,7 +312,10 @@ CAMLprim value stub_xc_domain_max_vcpus(value xch_val, value domid, xc_interface *xch = xch_of_val(xch_val); int r; + caml_enter_blocking_section(); r = xc_domain_max_vcpus(xch, Int_val(domid), Int_val(max_vcpus)); + caml_leave_blocking_section(); + if (r) failwith_xc(xch); @@ -329,7 +332,10 @@ value stub_xc_domain_sethandle(value xch_val, value domid, value handle) domain_handle_of_uuid_string(h, String_val(handle)); + caml_enter_blocking_section(); i = xc_domain_sethandle(xch, Int_val(domid), h); + caml_leave_blocking_section(); + if (i) failwith_xc(xch); @@ -391,7 +397,10 @@ CAMLprim value stub_xc_domain_shutdown(value xch_val, value domid, value reason) xc_interface *xch = xch_of_val(xch_val); int ret; + caml_enter_blocking_section(); ret = xc_domain_shutdown(xch, Int_val(domid), Int_val(reason)); + caml_leave_blocking_section(); + if (ret < 0) failwith_xc(xch); @@ -503,7 +512,10 @@ CAMLprim value stub_xc_domain_getinfo(value xch_val, value domid) xc_domaininfo_t info; int ret; + caml_enter_blocking_section(); ret = xc_domain_getinfo_single(xch, Int_val(domid), &info); + caml_leave_blocking_section(); + if (ret < 0) failwith_xc(xch); @@ -546,7 +558,10 @@ CAMLprim value stub_xc_vcpu_context_get(value xch_val, value domid, int ret; vcpu_guest_context_any_t ctxt; + caml_enter_blocking_section(); ret = xc_vcpu_getcontext(xch, Int_val(domid), Int_val(cpu), &ctxt); + caml_leave_blocking_section(); + if ( ret < 0 ) failwith_xc(xch); @@ -584,10 +599,14 @@ CAMLprim value stub_xc_vcpu_setaffinity(value xch_val, value domid, if (Bool_val(Field(cpumap, i))) c_cpumap[i/8] |= 1 << (i&7); } + + caml_enter_blocking_section(); retval = xc_vcpu_setaffinity(xch, Int_val(domid), Int_val(vcpu), c_cpumap, NULL, XEN_VCPUAFFINITY_HARD); + caml_leave_blocking_section(); + free(c_cpumap); if (retval < 0) @@ -612,10 +631,13 @@ CAMLprim value stub_xc_vcpu_getaffinity(value xch_val, value domid, if (c_cpumap == NULL) failwith_xc(xch); + caml_enter_blocking_section(); retval = xc_vcpu_getaffinity(xch, Int_val(domid), Int_val(vcpu), c_cpumap, NULL, XEN_VCPUAFFINITY_HARD); + caml_leave_blocking_section(); + if (retval < 0) { free(c_cpumap); failwith_xc(xch); @@ -639,9 +661,13 @@ CAMLprim value stub_xc_sched_id(value xch_val) { CAMLparam1(xch_val); xc_interface *xch = xch_of_val(xch_val); - int sched_id; + int ret, sched_id; + + caml_enter_blocking_section(); + ret = xc_sched_id(xch, &sched_id); + caml_leave_blocking_section(); - if (xc_sched_id(xch, &sched_id)) + if (ret) failwith_xc(xch); CAMLreturn(Val_int(sched_id)); @@ -674,7 +700,10 @@ CAMLprim value stub_xc_evtchn_reset(value xch_val, value domid) xc_interface *xch = xch_of_val(xch_val); int r; + caml_enter_blocking_section(); r = xc_evtchn_reset(xch, Int_val(domid)); + caml_leave_blocking_section(); + if (r < 0) failwith_xc(xch); CAMLreturn(Val_unit); @@ -811,7 +840,10 @@ CAMLprim value stub_xc_send_debug_keys(value xch_val, value keys) xc_interface *xch = xch_of_val(xch_val); int r; + caml_enter_blocking_section(); r = xc_send_debug_keys(xch, String_val(keys)); + caml_leave_blocking_section(); + if (r) failwith_xc(xch); CAMLreturn(Val_unit); @@ -952,7 +984,11 @@ CAMLprim value stub_xc_domain_set_memmap_limit(value xch_val, value domid, int retval; v = Int64_val(map_limitkb); + + caml_enter_blocking_section(); retval = xc_domain_set_memmap_limit(xch, Int_val(domid), v); + caml_leave_blocking_section(); + if (retval) failwith_xc(xch); @@ -1203,8 +1239,11 @@ CAMLprim value stub_xc_domain_ioport_permission(value xch_val, value domid, c_nr_ports = Int_val(nr_ports); c_allow = Bool_val(allow); + caml_enter_blocking_section(); ret = xc_domain_ioport_permission(xch, Int_val(domid), c_start_port, c_nr_ports, c_allow); + caml_leave_blocking_section(); + if (ret < 0) failwith_xc(xch); @@ -1225,8 +1264,11 @@ CAMLprim value stub_xc_domain_iomem_permission(value xch_val, value domid, c_nr_pfns = Nativeint_val(nr_pfns); c_allow = Bool_val(allow); + caml_enter_blocking_section(); ret = xc_domain_iomem_permission(xch, Int_val(domid), c_start_pfn, c_nr_pfns, c_allow); + caml_leave_blocking_section(); + if (ret < 0) failwith_xc(xch); @@ -1245,8 +1287,11 @@ CAMLprim value stub_xc_domain_irq_permission(value xch_val, value domid, c_pirq = Int_val(pirq); c_allow = Bool_val(allow); + caml_enter_blocking_section(); ret = xc_domain_irq_permission(xch, Int_val(domid), c_pirq, c_allow); + caml_leave_blocking_section(); + if (ret < 0) failwith_xc(xch); @@ -1309,7 +1354,9 @@ CAMLprim value stub_xc_domain_test_assign_device(value xch_val, value domid, val func = Int_val(Field(desc, 3)); sbdf = encode_sbdf(domain, bus, dev, func); + caml_enter_blocking_section(); ret = xc_test_assign_device(xch, Int_val(domid), sbdf); + caml_leave_blocking_section(); CAMLreturn(Val_bool(ret == 0)); } @@ -1328,8 +1375,10 @@ CAMLprim value stub_xc_domain_assign_device(value xch_val, value domid, value de func = Int_val(Field(desc, 3)); sbdf = encode_sbdf(domain, bus, dev, func); + caml_enter_blocking_section(); ret = xc_assign_device(xch, Int_val(domid), sbdf, XEN_DOMCTL_DEV_RDM_RELAXED); + caml_leave_blocking_section(); if (ret < 0) failwith_xc(xch); @@ -1350,7 +1399,9 @@ CAMLprim value stub_xc_domain_deassign_device(value xch_val, value domid, value func = Int_val(Field(desc, 3)); sbdf = encode_sbdf(domain, bus, dev, func); + caml_enter_blocking_section(); ret = xc_deassign_device(xch, Int_val(domid), sbdf); + caml_leave_blocking_section(); if (ret < 0) failwith_xc(xch); @@ -1379,8 +1430,11 @@ CAMLprim value stub_xc_get_cpu_featureset(value xch_val, value idx) /* To/from hypervisor to retrieve actual featureset */ uint32_t fs[fs_len], len = fs_len; unsigned int i; + int ret; - int ret = xc_get_cpu_featureset(xch, Int_val(idx), &len, fs); + caml_enter_blocking_section(); + ret = xc_get_cpu_featureset(xch, Int_val(idx), &len, fs); + caml_leave_blocking_section(); if (ret) failwith_xc(xch); @@ -1403,7 +1457,10 @@ CAMLprim value stub_xc_watchdog(value xch_val, value domid, value timeout) int ret; unsigned int c_timeout = Int32_val(timeout); + caml_enter_blocking_section(); ret = xc_watchdog(xch, Int_val(domid), c_timeout); + caml_leave_blocking_section(); + if (ret < 0) failwith_xc(xch);
We should be doing this unilaterally. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Christian Lindig <christian.lindig@citrix.com> CC: David Scott <dave@recoil.org> CC: Edwin Török <edwin.torok@cloud.com> CC: Rob Hoes <Rob.Hoes@citrix.com> CC: Andrii Sultanov <andrii.sultanov@cloud.com> Also pulled out of a larger cleanup series. --- tools/ocaml/libs/xc/xenctrl_stubs.c | 63 +++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 3 deletions(-)