| Message ID | 63240681327af995df64b773945406b097a31d4b.1669829264.git.edvin.torok@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OCaml bindings for hvm_param_get and xc_evtchn_status | expand |
On 30/11/2022 17:32, Edwin Török wrote: > diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli > index 60e7902e66..f6c7e5b553 100644 > --- a/tools/ocaml/libs/xc/xenctrl.mli > +++ b/tools/ocaml/libs/xc/xenctrl.mli > @@ -236,6 +236,51 @@ external map_foreign_range : > handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface > = "stub_map_foreign_range" > > +(* needs to be sorted according to its numeric value, watch out for gaps! *) > +type hvm_param = > + | HVM_PARAM_CALLBACK_IRQ > + | HVM_PARAM_STORE_PFN > + | HVM_PARAM_STORE_EVTCHN > + | HVM_PARAM_UNDEFINED_3 Can we perhaps use | _HVM_PARAM_UNDEF_3 with a leading underscore to highlight that its just a placeholder for a hole ? > + | HVM_PARAM_PAE_ENABLED > + | HVM_PARAM_IOREQ_PFN > + | HVM_PARAM_BUFIOREQ_PFN > + | HVM_PARAM_UNDEFINED_7 > + | HVM_PARAM_UNDEFINED_8 > + | HVM_PARAM_VIRIDIAN > + | HVM_PARAM_TIMER_MODE0 From TIMER_MODE onwards, you appear to have a trailing digit on each constant name. It appears to be the final digit of the params numeric value. > + | HVM_PARAM_HPET_ENABLED1 > + | HVM_PARAM_IDENT_PT2 > + | HVM_PARAM_UNDEFINED_13 > + | HVM_PARAM_ACPI_S_STATE4 > + | HVM_PARAM_VM86_TSS5 > + | HVM_PARAM_VPT_ALIGN6 > + | HVM_PARAM_CONSOLE_PFN7 > + | HVM_PARAM_CONSOLE_EVTCHN8 > + | HVM_PARAM_ACPI_IOPORTS_LOCATION9 > + | HVM_PARAM_MEMORY_EVENT_CR00 > + | HVM_PARAM_MEMORY_EVENT_CR31 > + | HVM_PARAM_MEMORY_EVENT_CR42 > + | HVM_PARAM_MEMORY_EVENT_INT33 > + | HVM_PARAM_NESTEDHVM4 > + | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5 > + | HVM_PARAM_UNDEFINED_26 > + | HVM_PARAM_PAGING_RING_PFN7 > + | HVM_PARAM_MONITOR_RING_PFN8 > + | HVM_PARAM_SHARING_RING_PFN9 > + | HVM_PARAM_MEMORY_EVENT_MSR0 > + | HVM_PARAM_TRIPLE_FAULT_REASON1 > + | HVM_PARAM_IOREQ_SERVER_PFN2 > + | HVM_PARAM_NR_IOREQ_SERVER_PAGES3 > + | HVM_PARAM_VM_GENERATION_ID_ADDR4 > + | HVM_PARAM_ALTP2M5 > + | HVM_PARAM_X87_FIP_WIDTH6 > + | HVM_PARAM_VM86_TSS_SIZED7 > + | HVM_PARAM_MCA_CAP8 Similarly with EVENTSTAT_*, we ought to engage the build time ABI check. But there isn't a suitable end delimiter, and these are only ever an input into a binding (never a return), so it's not the end of the world if new constants get missed. (Not that new constants are likely. HVM_PARAMs are a gross bodge which I'm trying to phase out.) > + > +external hvm_param_get: handle -> domid -> hvm_param -> int64 > + = "stub_xc_hvm_param_get" IMO we should bind set at the same time. It's trivial to do, and far easier to do now than at some point in the future when we first need it. > + > external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit > = "stub_xc_domain_assign_device" > external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit > diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c > index 67f3648391..b2df93d4f8 100644 > --- a/tools/ocaml/libs/xc/xenctrl_stubs.c > +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c > @@ -1176,6 +1176,22 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid, > CAMLreturn(Val_unit); > } > > +CAMLprim value stub_xc_hvm_param_get(value xch, value domid, value param) > +{ > + CAMLparam3(xch, domid, param); > + uint64_t result; result is commonly a value in these bindings. 'val' would be the more common name to use here. ~Andrew
> On 1 Dec 2022, at 11:51, Andrew Cooper <Andrew.Cooper3@citrix.com> wrote: > > On 30/11/2022 17:32, Edwin Török wrote: >> diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli >> index 60e7902e66..f6c7e5b553 100644 >> --- a/tools/ocaml/libs/xc/xenctrl.mli >> +++ b/tools/ocaml/libs/xc/xenctrl.mli >> @@ -236,6 +236,51 @@ external map_foreign_range : >> handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface >> = "stub_map_foreign_range" >> >> +(* needs to be sorted according to its numeric value, watch out for gaps! *) >> +type hvm_param = >> + | HVM_PARAM_CALLBACK_IRQ >> + | HVM_PARAM_STORE_PFN >> + | HVM_PARAM_STORE_EVTCHN >> + | HVM_PARAM_UNDEFINED_3 > > Can we perhaps use > > | _HVM_PARAM_UNDEF_3 > > with a leading underscore to highlight that its just a placeholder for a > hole ? I tried this, but I get a compile error if I attempt to start a variant name with and underscore. > >> + | HVM_PARAM_PAE_ENABLED >> + | HVM_PARAM_IOREQ_PFN >> + | HVM_PARAM_BUFIOREQ_PFN >> + | HVM_PARAM_UNDEFINED_7 >> + | HVM_PARAM_UNDEFINED_8 >> + | HVM_PARAM_VIRIDIAN >> + | HVM_PARAM_TIMER_MODE0 > > From TIMER_MODE onwards, you appear to have a trailing digit on each > constant name. It appears to be the final digit of the params numeric > value. > I think I see how that happened (I had the numbers side by side to check that I filled in all the wholes, and then used the wrong regex to remove them, which worked on single digit numbers, but not double). I'm fixing this up in my tree now. >> + | HVM_PARAM_HPET_ENABLED1 >> + | HVM_PARAM_IDENT_PT2 >> + | HVM_PARAM_UNDEFINED_13 >> + | HVM_PARAM_ACPI_S_STATE4 >> + | HVM_PARAM_VM86_TSS5 >> + | HVM_PARAM_VPT_ALIGN6 >> + | HVM_PARAM_CONSOLE_PFN7 >> + | HVM_PARAM_CONSOLE_EVTCHN8 >> + | HVM_PARAM_ACPI_IOPORTS_LOCATION9 >> + | HVM_PARAM_MEMORY_EVENT_CR00 >> + | HVM_PARAM_MEMORY_EVENT_CR31 >> + | HVM_PARAM_MEMORY_EVENT_CR42 >> + | HVM_PARAM_MEMORY_EVENT_INT33 >> + | HVM_PARAM_NESTEDHVM4 >> + | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5 >> + | HVM_PARAM_UNDEFINED_26 >> + | HVM_PARAM_PAGING_RING_PFN7 >> + | HVM_PARAM_MONITOR_RING_PFN8 >> + | HVM_PARAM_SHARING_RING_PFN9 >> + | HVM_PARAM_MEMORY_EVENT_MSR0 >> + | HVM_PARAM_TRIPLE_FAULT_REASON1 >> + | HVM_PARAM_IOREQ_SERVER_PFN2 >> + | HVM_PARAM_NR_IOREQ_SERVER_PAGES3 >> + | HVM_PARAM_VM_GENERATION_ID_ADDR4 >> + | HVM_PARAM_ALTP2M5 >> + | HVM_PARAM_X87_FIP_WIDTH6 >> + | HVM_PARAM_VM86_TSS_SIZED7 >> + | HVM_PARAM_MCA_CAP8 > > Similarly with EVENTSTAT_*, we ought to engage the build time ABI check. It looks like we'd need to write a new ABI check, but I'm not familiar with the ABI checker script here, and relying on a script to parse the OCaml source code and check the ABI seems brittle (but no less brittle than not having a check at all). Should we instead switch to using ctypes to generate these constants? It can run at build time and produce a .ml file based on a build time test by including the actual C header and getting the correct constant value. But it'd make cross-compilation (if Xen supports that?) more difficult. Or we could run it ourselves by hand, and commit the result so that end-users do not need to have or run ctypes, just developers who change these bindings (similar to how it is usual to commit the output from autoconf and automake into git to not require end-users to rerun these). However a move to ctypes would require quite a lot of build time changes that I'd rather start only once we switched to Dune, it is not worthwhile doing in the current Makefile based build system. > > But there isn't a suitable end delimiter, and these are only ever an > input into a binding (never a return), so it's not the end of the world > if new constants get missed. (Not that new constants are likely. > HVM_PARAMs are a gross bodge which I'm trying to phase out.) > >> + >> +external hvm_param_get: handle -> domid -> hvm_param -> int64 >> + = "stub_xc_hvm_param_get" > > IMO we should bind set at the same time. It's trivial to do, and far > easier to do now than at some point in the future when we first need it. > >> + >> external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit >> = "stub_xc_domain_assign_device" >> external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit >> diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c >> index 67f3648391..b2df93d4f8 100644 >> --- a/tools/ocaml/libs/xc/xenctrl_stubs.c >> +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c >> @@ -1176,6 +1176,22 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid, >> CAMLreturn(Val_unit); >> } >> >> +CAMLprim value stub_xc_hvm_param_get(value xch, value domid, value param) >> +{ >> + CAMLparam3(xch, domid, param); >> + uint64_t result; > > result is commonly a value in these bindings. 'val' would be the more > common name to use here. > > ~Andrew
> On 1 Dec 2022, at 13:50, Edwin Torok <edvin.torok@citrix.com> wrote: > > Should we instead switch to using ctypes to generate these constants? I would not advocate this. Ctypes is the kind of meta programming that is great when it works but hell if it does not and it adds more dependencies. I just had a discussion with Andrew about other tricks how to bring C constants to the ML side in order to decouple them. I’m using it in my Polly library - it might not be the solution for Xen but worth knowing. https://github.com/lindig/polly/blob/master/lib/polly_stubs.c#L23-L39 — C
> On 1 Dec 2022, at 13:57, Christian Lindig <christian.lindig@citrix.com> wrote: > > > >> On 1 Dec 2022, at 13:50, Edwin Torok <edvin.torok@citrix.com> wrote: >> >> Should we instead switch to using ctypes to generate these constants? > > I would not advocate this. Ctypes is the kind of meta programming that is great when it works but hell if it does not and it adds more dependencies. Perhaps use it to just generate the constant mappings? Here is an example of how I used it elsewhere: https://github.com/xapi-project/ocaml-dlm/blob/master/lib_gen/types/bindings_structs.ml#L30-L55 > > I just had a discussion with Andrew about other tricks how to bring C constants to the ML side in order to decouple them. I’m using it in my Polly library - it might not be the solution for Xen but worth knowing. > > https://github.com/lindig/polly/blob/master/lib/polly_stubs.c#L23-L39 The disadvantage is that we can't pattern match on it anymore. Although we could have some OCaml code that does the pattern matching on another type and maps it to these private integer types. However at that point we've manually reinvented what ctypes would already do, and we have an additional mapping step (which may not matter from a performance point of view but would be nice if we could avoid it). I'll have to do some experiments to see how the code generated by ctypes looks like in this case (actually the 'cstubs' part of it), and how different it would be from manually writing it (i.e. can we still reasonably review the generated code, and would it look like something that we'd write ourselves?) Best regards, --Edwin
> On 1 Dec 2022, at 14:16, Edwin Torok <edvin.torok@citrix.com> wrote: > > The disadvantage is that we can't pattern match on it anymore. > > Although we could have some OCaml code that does the pattern matching on another type and maps it to these private integer types. > However at that point we've manually reinvented what ctypes would already do, and we have an additional mapping step (which may not matter from a performance point of view but would be nice if we could avoid it). I agree that this is a severe disadvantage. My method is only useful if they are mostly passed around but not when they have an algebra defined over them where you want to combine and match them. — C
> On 1 Dec 2022, at 14:22, Christian Lindig <christian.lindig@citrix.com> wrote: > > > >> On 1 Dec 2022, at 14:16, Edwin Torok <edvin.torok@citrix.com> wrote: >> >> The disadvantage is that we can't pattern match on it anymore. >> >> Although we could have some OCaml code that does the pattern matching on another type and maps it to these private integer types. >> However at that point we've manually reinvented what ctypes would already do, and we have an additional mapping step (which may not matter from a performance point of view but would be nice if we could avoid it). > > I agree that this is a severe disadvantage. My method is only useful if they are mostly passed around but not when they have an algebra defined over them where you want to combine and match them. It might be possible to use your method to implement a pure-OCaml ABI checker though. Consider: ``` external constant_A : unit -> int = "caml_constant_A" external constant_B : unit -> int = "caml_constant_B" external constant_C : unit -> int = "caml_constant_C" let check_match = List.iter @@ fun (ocaml_variant, c_constant) -> let r = Obj.repr ocaml_variant in assert (Obj.is_int r); assert ((Obj.magic r : int) = c_constant ()) type t = A | B | C let () = [A, constant_A ;B, constant_B ;C, constant_C] |> check_match ``` (although perhaps with the 2nd assertion replaced by something that includes the constant in the exception raised to aid debugging) This'd only detect the ABI mismatch at runtime (although it would detect it right on startup), so it'd need to be accompanied by a small unit test that would link just this module to make any mismatches a build time failure. Then there would be no runtime overhead when using these variants in function calls, and we'd know they match 1:1. Although there is still a possibility of mismatching on names (the bug I originally had in my patch, which although wouldn't cause any runtime issues, would be good to catch at build time too). I'll keep thinking about this to see whether there is another easy approach we can use that doesn't require using Cstubs and doesn't rely on manually keeping code in different files in sync. Best regards, --Edwin
diff --git a/tools/ocaml/libs/xc/xenctrl.ml b/tools/ocaml/libs/xc/xenctrl.ml index c21e391f98..1f8d927b0c 100644 --- a/tools/ocaml/libs/xc/xenctrl.ml +++ b/tools/ocaml/libs/xc/xenctrl.ml @@ -298,6 +298,50 @@ external map_foreign_range: handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface = "stub_map_foreign_range" +type hvm_param = + | HVM_PARAM_CALLBACK_IRQ + | HVM_PARAM_STORE_PFN + | HVM_PARAM_STORE_EVTCHN + | HVM_PARAM_UNDEFINED_3 + | HVM_PARAM_PAE_ENABLED + | HVM_PARAM_IOREQ_PFN + | HVM_PARAM_BUFIOREQ_PFN + | HVM_PARAM_UNDEFINED_7 + | HVM_PARAM_UNDEFINED_8 + | HVM_PARAM_VIRIDIAN + | HVM_PARAM_TIMER_MODE0 + | HVM_PARAM_HPET_ENABLED1 + | HVM_PARAM_IDENT_PT2 + | HVM_PARAM_UNDEFINED_13 + | HVM_PARAM_ACPI_S_STATE4 + | HVM_PARAM_VM86_TSS5 + | HVM_PARAM_VPT_ALIGN6 + | HVM_PARAM_CONSOLE_PFN7 + | HVM_PARAM_CONSOLE_EVTCHN8 + | HVM_PARAM_ACPI_IOPORTS_LOCATION9 + | HVM_PARAM_MEMORY_EVENT_CR00 + | HVM_PARAM_MEMORY_EVENT_CR31 + | HVM_PARAM_MEMORY_EVENT_CR42 + | HVM_PARAM_MEMORY_EVENT_INT33 + | HVM_PARAM_NESTEDHVM4 + | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5 + | HVM_PARAM_UNDEFINED_26 + | HVM_PARAM_PAGING_RING_PFN7 + | HVM_PARAM_MONITOR_RING_PFN8 + | HVM_PARAM_SHARING_RING_PFN9 + | HVM_PARAM_MEMORY_EVENT_MSR0 + | HVM_PARAM_TRIPLE_FAULT_REASON1 + | HVM_PARAM_IOREQ_SERVER_PFN2 + | HVM_PARAM_NR_IOREQ_SERVER_PAGES3 + | HVM_PARAM_VM_GENERATION_ID_ADDR4 + | HVM_PARAM_ALTP2M5 + | HVM_PARAM_X87_FIP_WIDTH6 + | HVM_PARAM_VM86_TSS_SIZED7 + | HVM_PARAM_MCA_CAP8 + +external hvm_param_get: handle -> domid -> hvm_param -> int64 + = "stub_xc_hvm_param_get" + external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit = "stub_xc_domain_assign_device" external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit diff --git a/tools/ocaml/libs/xc/xenctrl.mli b/tools/ocaml/libs/xc/xenctrl.mli index 60e7902e66..f6c7e5b553 100644 --- a/tools/ocaml/libs/xc/xenctrl.mli +++ b/tools/ocaml/libs/xc/xenctrl.mli @@ -236,6 +236,51 @@ external map_foreign_range : handle -> domid -> int -> nativeint -> Xenmmap.mmap_interface = "stub_map_foreign_range" +(* needs to be sorted according to its numeric value, watch out for gaps! *) +type hvm_param = + | HVM_PARAM_CALLBACK_IRQ + | HVM_PARAM_STORE_PFN + | HVM_PARAM_STORE_EVTCHN + | HVM_PARAM_UNDEFINED_3 + | HVM_PARAM_PAE_ENABLED + | HVM_PARAM_IOREQ_PFN + | HVM_PARAM_BUFIOREQ_PFN + | HVM_PARAM_UNDEFINED_7 + | HVM_PARAM_UNDEFINED_8 + | HVM_PARAM_VIRIDIAN + | HVM_PARAM_TIMER_MODE0 + | HVM_PARAM_HPET_ENABLED1 + | HVM_PARAM_IDENT_PT2 + | HVM_PARAM_UNDEFINED_13 + | HVM_PARAM_ACPI_S_STATE4 + | HVM_PARAM_VM86_TSS5 + | HVM_PARAM_VPT_ALIGN6 + | HVM_PARAM_CONSOLE_PFN7 + | HVM_PARAM_CONSOLE_EVTCHN8 + | HVM_PARAM_ACPI_IOPORTS_LOCATION9 + | HVM_PARAM_MEMORY_EVENT_CR00 + | HVM_PARAM_MEMORY_EVENT_CR31 + | HVM_PARAM_MEMORY_EVENT_CR42 + | HVM_PARAM_MEMORY_EVENT_INT33 + | HVM_PARAM_NESTEDHVM4 + | HVM_PARAM_MEMORY_EVENT_SINGLE_STEP5 + | HVM_PARAM_UNDEFINED_26 + | HVM_PARAM_PAGING_RING_PFN7 + | HVM_PARAM_MONITOR_RING_PFN8 + | HVM_PARAM_SHARING_RING_PFN9 + | HVM_PARAM_MEMORY_EVENT_MSR0 + | HVM_PARAM_TRIPLE_FAULT_REASON1 + | HVM_PARAM_IOREQ_SERVER_PFN2 + | HVM_PARAM_NR_IOREQ_SERVER_PAGES3 + | HVM_PARAM_VM_GENERATION_ID_ADDR4 + | HVM_PARAM_ALTP2M5 + | HVM_PARAM_X87_FIP_WIDTH6 + | HVM_PARAM_VM86_TSS_SIZED7 + | HVM_PARAM_MCA_CAP8 + +external hvm_param_get: handle -> domid -> hvm_param -> int64 + = "stub_xc_hvm_param_get" + external domain_assign_device: handle -> domid -> (int * int * int * int) -> unit = "stub_xc_domain_assign_device" external domain_deassign_device: handle -> domid -> (int * int * int * int) -> unit diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c index 67f3648391..b2df93d4f8 100644 --- a/tools/ocaml/libs/xc/xenctrl_stubs.c +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c @@ -1176,6 +1176,22 @@ CAMLprim value stub_xc_domain_irq_permission(value xch, value domid, CAMLreturn(Val_unit); } +CAMLprim value stub_xc_hvm_param_get(value xch, value domid, value param) +{ + CAMLparam3(xch, domid, param); + uint64_t result; + int ret; + + caml_enter_blocking_section(); + ret = xc_hvm_param_get(_H(xch), _D(domid), Int_val(param), &result); + caml_leave_blocking_section(); + + if ( ret ) + failwith_xc(_H(xch)); + + CAMLreturn(caml_copy_int64(result)); +} + static uint32_t encode_sbdf(int domain, int bus, int dev, int func) { return ((uint32_t)domain & 0xffff) << 16 |
Not to be confused which hvm_get_param, which also exists and has a different, more error-prone interface. This one always returns a 64-bit value, and that is retained in the OCaml binding as well, returning 'int64' (and not int, or nativeint which might have a different size). The integer here is unsigned in the C API, however OCaml only has signed integers. No bits are lost, it is just a matter of interpretation when printing and for certain arithmetic operations, however in the cases where the MSB is set it is very likely that the value is an address and no arithmetic should be performed on the OCaml side on it. (this is not a new problem with this binding, but worth mentioning given the difference in types) Signed-off-by: Edwin Török <edvin.torok@citrix.com> --- tools/ocaml/libs/xc/xenctrl.ml | 44 ++++++++++++++++++++++++++++ tools/ocaml/libs/xc/xenctrl.mli | 45 +++++++++++++++++++++++++++++ tools/ocaml/libs/xc/xenctrl_stubs.c | 16 ++++++++++ 3 files changed, 105 insertions(+)