Re: [PATCH v1 3/5] tools/ocaml/libs/xc: add hvm_param_get binding

[Edwin Torok <edvin.torok@xxxxxxxxxx>]

> On 1 Dec 2022, at 11:51, Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx> 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